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
fix(ivy): i18n - support setting locales for each translation file #33381
fix(ivy): i18n - support setting locales for each translation file #33381
Conversation
packages/localize/src/tools/src/translate/translation_files/translation_file_loader.ts
Outdated
Show resolved
Hide resolved
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.
the rest lgtm
...localize/src/tools/src/translate/translation_files/translation_parsers/translation_parser.ts
Outdated
Show resolved
Hide resolved
...ools/src/translate/translation_files/translation_parsers/xliff1/xliff1_translation_parser.ts
Outdated
Show resolved
Hide resolved
...c/tools/test/translate/translation_files/translation_parsers/simple_json/simple_json_spec.ts
Outdated
Show resolved
Hide resolved
9af849e
to
1850ad9
Compare
1850ad9
to
28fd971
Compare
After @clydin's feedback I completely redesigned this PR - so all the files have changed since the last reviews (cc @IgorMinar). @clydin please note that the return type from PTAL!! |
28fd971
to
646302c
Compare
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 few more suggestions.
if (parsedLocale !== undefined && providedLocale !== undefined && | ||
parsedLocale !== providedLocale) { | ||
this.diagnostics.warn( | ||
`The provided locale "${providedLocale}" and the target locale "${parsedLocale}" do not match for translation file "${filePath}".`); |
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.
I don't think that this is easy to understand. How about "specified locale XX doesn't match locale YY found in the translation file"
const loader = new TranslationLoader([parser], diagnostics); | ||
expect(() => loader.loadBundles(['/src/locale/messages.en.xlf'], [])) | ||
.toThrowError( | ||
'No target locale provided nor found in the translation file "/src/locale/messages.en.xlf"'); |
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.
Maybe add "please provide --target-locale argument"?
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.
The only problem with this is that it might be called via the Angular CLI which does not have this option...
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.
How about: "consider explicitly providing a target-locale for this file"?
*/ | ||
export interface ParsedTranslationBundle { | ||
parsedLocale: string|undefined; | ||
parsedTranslations: Record<ɵMessageId, ɵParsedTranslation>; |
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.
If these names are kept unchanged, no code changes would be needed on the CLI side.
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.
This is where it is used in the CLI (for context): https://github.com/angular/angular-cli/blob/395c97912800af435021432e708cbdfab28783fe/packages/angular_devkit/build_angular/src/utils/load-translations.ts
Only the translations
property appears to be important...
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.
I'll switch them back...
d742d1c
to
529d94c
Compare
As I am a code-owner of this, and having approval from @IgorMinar, I think that this can be merged, right? |
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.
LGTM, just left a couple comments. Thanks Pete.
...c/tools/test/translate/translation_files/translation_parsers/simple_json/simple_json_spec.ts
Outdated
Show resolved
Hide resolved
...c/tools/test/translate/translation_files/translation_parsers/simple_json/simple_json_spec.ts
Outdated
Show resolved
Hide resolved
|
||
diagnostics.messages.forEach(m => console.warn(`${m.type}: ${m.message}`)); | ||
process.exit(diagnostics.hasErrors ? 1 : 0); | ||
} | ||
|
||
export interface TranslateFilesOptions { | ||
/** |
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.
Thanks for adding docs 👍
Previously the target locale of a translation file had to be extracted from the contents of the translation file. Therefore it was an error if the translation file did not provide a target locale. Now an array of locales can be provided via the `translationFileLocales` option that overrides any target locale extracted from the file. This allows us to support translation files that do not have a target locale specified in their contents. // FW-1644 Fixes angular#33323
The `localize-translate` command line tool can now accept an array of target locales to support the case where translation files do not contain them. Specify this array via the `--target-locales` option. NOTE to early adopters: in order to support this, the original `-t` option for the binary has changed from being a glob pattern to an array of paths, which will have matching indices to any provided target-locales.
529d94c
to
c7a3767
Compare
…#33381) The `localize-translate` command line tool can now accept an array of target locales to support the case where translation files do not contain them. Specify this array via the `--target-locales` option. NOTE to early adopters: in order to support this, the original `-t` option for the binary has changed from being a glob pattern to an array of paths, which will have matching indices to any provided target-locales. PR Close #33381
…ngular#33381) Previously the target locale of a translation file had to be extracted from the contents of the translation file. Therefore it was an error if the translation file did not provide a target locale. Now an array of locales can be provided via the `translationFileLocales` option that overrides any target locale extracted from the file. This allows us to support translation files that do not have a target locale specified in their contents. // FW-1644 Fixes angular#33323 PR Close angular#33381
…angular#33381) The `localize-translate` command line tool can now accept an array of target locales to support the case where translation files do not contain them. Specify this array via the `--target-locales` option. NOTE to early adopters: in order to support this, the original `-t` option for the binary has changed from being a glob pattern to an array of paths, which will have matching indices to any provided target-locales. PR Close angular#33381
…ngular#33381) Previously the target locale of a translation file had to be extracted from the contents of the translation file. Therefore it was an error if the translation file did not provide a target locale. Now an array of locales can be provided via the `translationFileLocales` option that overrides any target locale extracted from the file. This allows us to support translation files that do not have a target locale specified in their contents. // FW-1644 Fixes angular#33323 PR Close angular#33381
…angular#33381) The `localize-translate` command line tool can now accept an array of target locales to support the case where translation files do not contain them. Specify this array via the `--target-locales` option. NOTE to early adopters: in order to support this, the original `-t` option for the binary has changed from being a glob pattern to an array of paths, which will have matching indices to any provided target-locales. PR Close angular#33381
This issue has been automatically locked due to inactivity. Read more about our automatic conversation locking policy. This action has been performed automatically by a bot. |
Previously the target locale of a translation file had to be extracted
from the contents of the translation file.
Now an array of locales can be provided via the
translationFileLocales
option that overrides any target locale extracted from the file.
This allows us to support translation files that do not have a target
locale specified in their contents.
// FW-1644
Fixes #33323