-
Notifications
You must be signed in to change notification settings - Fork 57
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
Context implemenation and some test cases #116
base: master
Are you sure you want to change the base?
Conversation
- if domain is mentioned it is ignored as it is not used in extraction.
I have added implementation of |
Wow, that's nice! Thanks! |
Any chance this could go into next release? Thanks. |
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 a lot for the hard work! The code just needs some clean up and after that I think we're good to go.
lib/jsxgettext.js
Outdated
} | ||
|
||
// Always add the funcName as last element in return array | ||
// If the gettext function's name starts with "np" (i.e. npgettext or np_) and its 3 arguments are strings, we regard it as 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.
Is there documentation around this for existing gettext toolchain? If yes, it would be great to reference that here. If not, I'd like to hear more about your reasoning :)
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 used GNU gettext as reference. npgettext is mentioned here. node-gettext implements this. Should I reference this here?
lib/jsxgettext.js
Outdated
|
||
return [arg, node.arguments[1], funcName]; | ||
|
||
// If the gettext function's name starts with "p" (i.e. pgettext or p_) and its 2 arguments are strings, we regard it as 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.
There's now a lot of repetition around argument type checks here so I think we should move them into their own function. Something like:
function areArgsString(args) {
// magic
return wand;
}
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.
Something like
// check if the args (object or array of objects) are
function areArgsString(args) {
return args && [].concat(args).every(function (arg) {
return arg && (isStringLiteral(arg) || isStrConcatExpr(arg))
});
}
And we call it like
if (funcName.substr(0, 2) === "np" && areArgsString(node.arguments.slice(0, 3)))
lib/jsxgettext.js
Outdated
@@ -143,7 +159,7 @@ function parse(sources, options) { | |||
|
|||
// Always use the default context for now |
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.
Comment needs updating?
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.
Ok. Should remove this as we are using context now.
lib/jsxgettext.js
Outdated
return; | ||
|
||
var funcName = args.pop(); |
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 think this logic should be isolated into getTranslatable
. It should only return a structure with the fields context
and translatable
.
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.
getTranslatable
will return [context, translatable]
lib/jsxgettext.js
Outdated
if (comments.extracted) | ||
comments.extracted = comments.extracted.split('\n').filter(dedupeNCoalesce).join('\n'); | ||
Object.keys(translations).forEach(function (msgctxt) { | ||
Object.keys(translations[msgctxt]).forEach(function (msgid){ |
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 you extract the inner function definition to outside of the forEach
iterations and then pass msgctxt
as the "context" argument to the inner forEach
iteration? This way you can avoid redefining the inner function for each context iteration.
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.
Something like
function extractComments(msgid) {
if (!this[msgid].comments)
return;
if (this[msgid].comments.reference)
this[msgid].comments.reference = this[msgid].comments.reference.split('\n').filter(dedupeNCoalesce).join('\n');
if (this[msgid].comments.extracted)
this[msgid].comments.extracted = this[msgid].comments.extracted.split('\n').filter(dedupeNCoalesce).join('\n');
}
Object.keys(translations).forEach(function (msgctxt) {
Object.keys(translations[msgctxt]).forEach(extractComments, translations[msgctxt]);
});
test/inputs/anonymous_functions.js
Outdated
testObj.ngettext("I'm also gonna get translated!", "I'm the plural form!", 2); | ||
testObj.pgettext("context1", "I am translated in 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.
I'm not sure this reflects the real-world context usage since I expect them to come from a variable usually. What do you think? (I know if they are coming from a variable it is impossible to extract them as we do here in a static analysis)
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.
That looks like a typeo. It was correct before
testObj.gettext("I'm gonna get translated, yay!")
@@ -157,7 +173,7 @@ function parse(sources, options) { | |||
}); |
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.
Should add other keywords to aliases
if (options.keyword) {
Object.keys(options.keyword).forEach(function (index) {
['n', 'p', 'np', 'd', 'dn', 'dp', 'dnp'].forEach(function (keyword) {
options.keyword.push(keyword + options.keyword[index]);
});
});
}
Hi I have made the requested changes. Please let me know if there's more changes needed. |
This is the implementation of
pgettext
andnpgettext
mentioned in #95.I have added some test cases for
pgettext
andnpgettext
. Other tests, that I have not modified, are passing the tests as well.Please let me know if you have any comments.