Issue 29 - Handle multiple YML files for translations #88
Conversation
amerharb
changed the title
| const output: MessageMap = {}; | ||
| Object.entries(language).forEach(([key, value]) => { | ||
| output[key] = value.text; | ||
| output[key] = value; | ||
| }); |
There was a problem hiding this comment.
This cloning is no longer necessary as language is already a MessageMap.
| de: { | ||
| 'greeting.headline': { | ||
| sourceFile: '', | ||
| sourceFile: 'de-with_extra_text.yaml', |
There was a problem hiding this comment.
A sourceFile under 'de' with a value not starting with 'de.' is unrealistic.
The first dot-separated segment of the last portion of the file's path is used as language identifier by the real YamlTranslationAdapter.
See also the test can update translations before getTranslations() below.
| expect(before).toEqual({ | ||
| 'greeting.headline': 'Hallo', | ||
| }); | ||
| expect(before).toEqual('Hallo'); | ||
|
|
||
| expect(after).toEqual({ | ||
| 'greeting.headline': 'Hallo!', | ||
| }); | ||
| expect(after).toEqual('Hallo!'); |
There was a problem hiding this comment.
The test also covered that updateTranslation does not add any other properties to the objects in the maps returned by getTranslations. TypeScript already enforces that so there is no problem with losing that coverage here.
This is just a note from my review that I thought might be interesting. You can resolve the conversation when you have read it.
| const sourceFileDotCount = sourceFile.replace('/', '.').split('.').length - 1; | ||
| const msgIdArray = msgId.split('.').splice(sourceFileDotCount); |
There was a problem hiding this comment.
This code overestimates how much of the message id comes from the path. The filename --- the last segment of the path, typically contains a dot, for example in 'sv.yml' but can in theory contain more dots too, for example 'sv.skånska.yaml'.
For example getShortId('a.b.c', 'a/sv.yml') should return 'b.c' but this implementation will return just 'c'.
| } | ||
|
|
||
| function getShortId(msgId: string, sourceFile: string): string { | ||
| const sourceFileDotCount = sourceFile.replace('/', '.').split('.').length - 1; |
There was a problem hiding this comment.
String.prototype.replace only replaces the first occurrence if pattern is a string.
For example getShortId('a.b.c.d', 'a/b/c/sv.yml') should return just 'd' but this implementation returns 'c.d'.
Fortunately, there is another function replaceAll that works as expected.
https://developer.mozilla.org/en-US/docs/Web/JavaScript/Reference/Global_Objects/String/replace
https://developer.mozilla.org/en-US/docs/Web/JavaScript/Reference/Global_Objects/String/replaceAll
| function getShortId(msgId: string, sourceFile: string): string { | ||
| const sourceFileDotCount = sourceFile.replace('/', '.').split('.').length - 1; | ||
| const msgIdArray = msgId.split('.').splice(sourceFileDotCount); | ||
| return msgIdArray.join('.'); | ||
| } |
There was a problem hiding this comment.
This function is complex enough that it should have a somewhat extensive test suite.
| @@ -53,32 +56,30 @@ describe('ProjectStore', () => { | |||
| getTranslations: async () => ({ | |||
| de: { | |||
| 'greeting.headline': { | |||
| sourceFile: '', | |||
| sourceFile: 'anything', | |||
There was a problem hiding this comment.
A sourceFile under locale identifier de with a value not starting with de. is unrealistic.
The first dot-separated segment of the last portion of the file's path is used as locale identifier by the real YamlTranslationAdapter.
| sourceFile: 'anything', | |
| sourceFile: 'de.yaml', |
No description provided.