-
Notifications
You must be signed in to change notification settings - Fork 4
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
feature: PEX-20271 replacements context #140
Conversation
a81eb93
to
b143daf
Compare
b143daf
to
31e1f49
Compare
@@ -19,6 +19,8 @@ const DEFAULT_HEADERS = { | |||
|
|||
const DEFAULT_ADD_LOCATION = 'full' | |||
|
|||
const NO_CONTEXT = 'no_context' |
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.
Issue: If I remember correctly the "empty" context should always be empty, if not explicitly given.
const NO_CONTEXT = 'no_context' | |
const NO_CONTEXT = '' |
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.
Issue: If I remember correctly the "empty" context should always be empty, if not explicitly given.
It is needed only for reading fields without the context from replacements.json. It won't appear in the message.pot
I described its purpose in another comment.
src/translate.js
Outdated
@@ -33,12 +24,28 @@ export default (singleton) => { | |||
finalOptions = { | |||
...defaultOptions, | |||
...finalOptions, | |||
replacementsContext: (finalOptions && finalOptions.context) || 'no_context', |
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.
Question: Wouldn't it be good to reuse the already existing instead of saving a secondary replacement context?
to my understanding the replacementContext is always the same as the original translation context?
Question 2: When thinking about contexts, wouldn't that be a good way to handle replacements without having the secondary JSON file?
I mean you could list 2 translations:
msgctxt "old"
msgid "go back"
msgstr "go back old
msgctxt "new"
msgid "go back"
msgstr "go back new"
msgid "hi"
msgstr "Hello"
And then pick the correct translations via the contexts instead of using custom replacement files for each applications.
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.
Question: Wouldn't it be good to reuse the already existing instead of saving a secondary replacement context?
The context itself is always the same, however, we need a way to specify it in the replacements.json
.
We can't use an empty string as a key for replacements without context because it is not a valid object key.
That's why there is a no_context
key in the replacements.json
for this case.
Question 2: When thinking about contexts, wouldn't that be a good way to handle replacements without having the secondary JSON file?
It won't fit because of two reasons:
- In that case the original context will be lost
- Translation team/service looks at the
msgid
of themessage.pot
. Having the samemsgid
will require to change the translation process for them to translate it to the different languages.
@@ -28,6 +28,8 @@ export default translate | |||
* locale | |||
* @param configObj A hashmap with keys `default` (default locale) and `map` (mapping of locales to | |||
* other locales) | |||
* @param replacements A hashmap with translation context keys and translation hashmaps as their values |
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.
Question With this change, should we bump the version in package.json
?
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.
@ThomasR I think we can bump the minor
version when we publish it.
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 would opt for minor as well
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.
@star4beam After reviewing I think this could be a semver major change since the way replacement json needs to be configured has changed thus making it not backwards compatible. WDYT?
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.
@Matwog Hmm, ok!
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 🚀
Problem
Replacement feature won't work properly with translation context
Solution
More info is available in readme