-
Notifications
You must be signed in to change notification settings - Fork 6
[DIT-11970] iOS Strings Format Support #129
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
[DIT-11970] iOS Strings Format Support #129
Conversation
…class, and IOSStringsOutput file
…riants and projects as expected. Moved shared BaseFormatter class methods out of JSON and IOSStringsFormatters
lib/src/formatters/iosStrings.ts
Outdated
| const components = await this.fetchComponents(); | ||
| const variables = await this.fetchVariables(); | ||
|
|
||
| const variablesById = variables.reduce((acc, variable) => { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Do we use this anywhere? I see we have a variable output file but am not seeing how that gets populated. In the base class maybe?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Yeah good question, the variables are a big ? for me too. Gonna dig into those now and will report back
…ponent mapping to iosStringsFormatter
… Unit tests additions to IOSStringsFormatter class
| * | ||
| * @returns {OutputFile[]} List of Output Files | ||
| */ | ||
| protected transformAPIData(data: IOSStringsAPIData) { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
A little weird to call these "formatters" now that the export endpoint will do all of the formatting and all this is doing is saving to a file. Maybe worth renaming when you do the refactor work later.
| params: PullQueryParams, | ||
| meta: CommandMetaFlags | ||
| ) { | ||
| function fetchTextWrapper<TResponse>(cb: () => Promise<TResponse>) { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
What was the purpose of the wrapper function here instead of just putting the switch statement directly in the try block here?
joustrich
left a comment
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Couple minor comments, but otherwise looks good!
Overview
IOSStringsFormatterto handleoutputswhereformat: ios-stringsBaseFormatterclass with shared format functionalityBaseFormatterclassGET /v2/textItems/exportandGET /v2/components/exportendpoint supportGET /v2/variantshttpGET /v2/projectshttppullcommand tests forios-stringsformat configContext
DIT-11970: [CLI] IOS Strings Format
Test Plan
Setup
.envlooks like the followingconfig.ymlin the./clirepository to organize formats intooutDirs. This is what mine looks likeAssertions
format: jsonwith noframeworkvariables.jsonfile at the root of the configuredoutDirwith your WS's variablesformat: jsonwith frameworki18nextindex.jsfile at the root of the configuredoutDirwith files exported as expectedios-stringsFormat works as expectedformat: ios-stringsoutDir(either at the project root config or in the output config)components___{variant}.stringsfile for each configured variant{project}___{variant}.stringsfile for each configured project/variant, e.g.variables.jsonfile. This is for typescript typing and not iOS stuff**im like 95% sure on this