-
Notifications
You must be signed in to change notification settings - Fork 9
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
MOB-76 Update logic for exporting copies for JS #63
base: pv/MOB-80-update-dependency-versions
Are you sure you want to change the base?
Changes from 3 commits
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
Original file line number | Diff line number | Diff line change |
---|---|---|
|
@@ -2,10 +2,11 @@ const path = require("path"); | |
const Result = require("../utils/result"); | ||
const { normalizeYaml, PLURAL, SINGULAR } = require("../actions/normalize"); | ||
const { loadFile } = require("../utils/fileUtils"); | ||
const { accessiblityKeywords, groupKeywords, outputType } = require("../model/keywords"); | ||
const { accessiblityKeywords, groupKeywords, outputTypes } = require("../model/keywords"); | ||
const { groupByKey } = require("../utils/arrayUtils"); | ||
const { flatten } = require("../utils/arrayUtils"); | ||
const Handlebars = require("handlebars"); | ||
const LodashUpdateWith = require("lodash.updatewith"); | ||
|
||
const percentEncodingPattern = /%(?!\d+{{.}})/; | ||
|
||
|
@@ -43,6 +44,7 @@ const renderLocalizationView = (view, outputType, language, basePath) => { | |
return localizationTemplate(outputType) | ||
.map((source) => Handlebars.compile(source)) | ||
.map((template) => template(view)) | ||
.map((renderedTemplate) => postTemplateRenderFormatting(renderedTemplate, outputType)) | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Instead of There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Then it's very specific. In this function, we could be doing different things based on output mode, There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more.
Then probably it's against the "single responsibility" principle :D There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Ain't it single resposibility, If the responsibility is "postProcessing", whatever that means, based on outputType? o_O There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. I agree with @pooyahrtn, let's be explicit about what the function is doing. If we ever need more complicated post-processing steps, we can always change the function name later on. |
||
.map((render) => ({ path: outputPath, data: render })); | ||
}; | ||
|
||
|
@@ -63,30 +65,45 @@ const renderCodeGenView = (view, outputType, basePath) => { | |
} | ||
}; | ||
|
||
const postTemplateRenderFormatting = (renderedTemplate, outputType) => { | ||
if (outputType === outputTypes.JS) { | ||
// Convert 1-on-1 key mapping to a JSON object | ||
const renderedTemplateAsJSON = JSON.parse(renderedTemplate); | ||
const localizationObject = {}; | ||
Object.keys(renderedTemplateAsJSON).forEach((key) => { | ||
LodashUpdateWith(localizationObject, key, () => renderedTemplateAsJSON[key], Object); | ||
}); | ||
|
||
return JSON.stringify(localizationObject, null, 2); | ||
} | ||
|
||
return renderedTemplate; | ||
}; | ||
|
||
const localizationTemplate = (type) => { | ||
switch (type) { | ||
case outputType.ANDROID: | ||
case outputTypes.ANDROID: | ||
return loadFile(path.resolve(__dirname, "../../templates/strings_xml_file.hbs")); | ||
case outputType.IOS: | ||
case outputTypes.IOS: | ||
return loadFile(path.resolve(__dirname, "../../templates/localizable_strings_file.hbs")); | ||
case outputType.JS: | ||
case outputTypes.JS: | ||
return loadFile(path.resolve(__dirname, "../../templates/json_strings_file.hbs")); | ||
} | ||
}; | ||
|
||
const codeGenerationTemplate = (type) => { | ||
switch (type) { | ||
case outputType.ANDROID: | ||
case outputTypes.ANDROID: | ||
return undefined; | ||
case outputType.IOS: | ||
case outputTypes.IOS: | ||
return loadFile( | ||
path.resolve(__dirname, "../../templates/code_generation_swift_file.hbs") | ||
).flatMap((fileTemplate) => | ||
loadFile( | ||
path.resolve(__dirname, "../../templates/code_generation_swift_child.hbs") | ||
).flatMap((childTemplate) => Result.success({ fileTemplate, childTemplate })) | ||
); | ||
case outputType.JS: | ||
case outputTypes.JS: | ||
return loadFile( | ||
path.resolve(__dirname, "../../templates/code_generation_js_file.hbs") | ||
).flatMap((fileTemplate) => Result.success({ fileTemplate, undefined })); | ||
|
@@ -103,29 +120,29 @@ const codeGenerationOutputPath = (basePath, outputType) => { | |
|
||
const localizationFileName = (type) => { | ||
switch (type) { | ||
case outputType.ANDROID: | ||
case outputTypes.ANDROID: | ||
return "strings.xml"; | ||
case outputType.IOS: | ||
case outputTypes.IOS: | ||
return "Localizable.strings"; | ||
case outputType.JS: | ||
case outputTypes.JS: | ||
return "strings.json"; | ||
} | ||
}; | ||
|
||
const codeGenerationFileName = (type) => { | ||
switch (type) { | ||
case outputType.ANDROID: | ||
case outputTypes.ANDROID: | ||
return "Localizable.kt"; | ||
case outputType.IOS: | ||
case outputTypes.IOS: | ||
return "Localizable.swift"; | ||
case outputType.JS: | ||
case outputTypes.JS: | ||
return "Localizable.ts"; | ||
} | ||
}; | ||
|
||
const substitutionsForOutputType = (type) => { | ||
switch (type) { | ||
case outputType.ANDROID: | ||
case outputTypes.ANDROID: | ||
// prettier-ignore | ||
return [ | ||
// Important: & should be substituted before we introduce new ampersands as part of our substitutions | ||
|
@@ -141,15 +158,15 @@ const substitutionsForOutputType = (type) => { | |
{ search: ">", replace: ">" }, | ||
{ search: "\"", replace: """ }, | ||
]; | ||
case outputType.IOS: | ||
case outputTypes.IOS: | ||
return [ | ||
// Important: % should be substituted before we substitute {{s}} and {{d}} | ||
{ search: percentEncodingPattern, replace: "%%" }, | ||
{ search: "{{s}}", replace: "$@" }, | ||
{ search: "{{d}}", replace: "$d" }, | ||
{ search: '"', replace: '\\"' }, | ||
]; | ||
case outputType.JS: | ||
case outputTypes.JS: | ||
return [ | ||
{ search: "{{s}}", replace: "$s" }, | ||
{ search: "{{d}}", replace: "$d" }, | ||
|
@@ -165,11 +182,11 @@ const substitutionsForOutputType = (type) => { | |
|
||
const keyDelimiterForOutputType = (type) => { | ||
switch (type) { | ||
case outputType.ANDROID: | ||
case outputTypes.ANDROID: | ||
return "_"; | ||
case outputType.IOS: | ||
case outputTypes.IOS: | ||
return "."; | ||
case outputType.JS: | ||
case outputTypes.JS: | ||
return "."; | ||
} | ||
}; | ||
|
Original file line number | Diff line number | Diff line change |
---|---|---|
|
@@ -15,7 +15,7 @@ const accessiblityKeywords = { | |
VALUE: "VALUE", | ||
}; | ||
|
||
const outputType = { | ||
const outputTypes = { | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. I'd say the previous name wasn't that bad, and I would rather keep it to make it easier later to read the git changes. There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. For these lines, outType (in global scope) & outType (in local scope) is confusing. That's why changed it to this. |
||
ANDROID: "ANDROID", | ||
IOS: "IOS", | ||
JS: "JS", | ||
|
@@ -29,7 +29,7 @@ const isPluralGroup = (group) => | |
module.exports = { | ||
groupKeywords, | ||
accessiblityKeywords, | ||
outputType, | ||
outputTypes, | ||
isLeafGroup, | ||
isPluralGroup, | ||
}; |
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 nit, shouldn't it be lowercase?