make -k other_keyword append to the defaults #82

jsut · 2021-03-26T18:59:48Z

This is how xgettext works, which is how the docs say it works.

smhg

Thank you, this is a great idea. I've left some individual line comments.
On top of that, I think 2 things are still missing:

xgettext allows to reset the default keywords by passing -k (without a word). It would be a very good idea to add this behavior in this PR since this provides the easiest upgrade path from the old usage. Otherwise things will break much more for existing users.
It would be great to have tests for the actual differences in keywords passed to the parsers. Now we only test the resulting output. This would require to expose (a part of) the current getParser function somehow. I don't know what the best approach is; feedback on this is very much appreciated.

smhg · 2021-03-27T06:27:40Z

index.js

@@ -91,10 +91,9 @@ function xgettext (input, options, cb) {
    if (!parsers[name]) {
      const Parser = require(`gettext-${name}`);

-      if (Object.keys(keywordSpec).length > 0) {
+      if (Parser.keywordSpec) {


If the parser does not offer a default, no keywordSpec is passed to the parser.

smhg · 2021-03-27T06:29:53Z

index.js

@@ -91,10 +91,9 @@ function xgettext (input, options, cb) {
    if (!parsers[name]) {
      const Parser = require(`gettext-${name}`);

-      if (Object.keys(keywordSpec).length > 0) {
+      if (Parser.keywordSpec) {
+        keywordSpec = Object.assign(keywordSpec, Parser.keywordSpec);


This parameter order of Object.assign always overwrites all custom keywords with their defaults.

smhg · 2021-03-27T06:31:56Z

test/fixtures/keyword.hbs

What's the change here?

vim being particular about whether their should be a newline at the end of the file. I'll fix it.

Strange, as I don't see a newline at the end of the file in master either. Can it be a different line ending (should be Unix-style)?

there was no newline before.

jsut · 2021-03-27T14:51:45Z

* `xgettext` allows to reset the default keywords by passing `-k` (without a word). It would be a very good idea to add this behavior in this PR since this provides the easiest upgrade path from the old usage. Otherwise things will break much more for existing users.

I haven't looked at the code for this yet, but it seems like another PR to me. There are other issues with -k as well. With gnu xgettext there doesn't have to be (and perhaps cannot be, i haven't tested it) a space after the -k before the identifier. I think another PR that adds this functionality and adds tests for createKeywordSpec could be the right thing to do.

* It would be great to have tests for the actual differences in keywords passed to the parsers. Now we only test the resulting output. This would require to expose (a part of) the current `getParser` function somehow. I don't know what the best approach is; feedback on this is very much appreciated.

Perhaps add more functionality to keyword-spec.js, a function that will take a bunch of specs and combine them. Then you could test that function directly, and the code currently in this PR gets even simpler.

smhg · 2021-03-27T15:16:13Z

I haven't looked at the code for this yet, but it seems like another PR to me. There are other issues with -k as well. With gnu xgettext there doesn't have to be (and perhaps cannot be, i haven't tested it) a space after the -k before the identifier. I think another PR that adds this functionality and adds tests for createKeywordSpec could be the right thing to do.

I prefer this in the same PR, but separate is fine too. It's just that this functionality is crucial for this PR to move ahead.
Someone who would install a version with the current change and uses -k somekeyword would suddenly also get all default keywords and quite realistically a much different output. There would be no migration path. When you can disable the defaults with -k, you'd only need to change this to -k -k somekeyword. The same way xgettext handles this.

You don't need to worry about the exact way to specify parameters as that's something that is handled by the optstring parser (yargs). We stick to their format, not xgettext's, in that area.

Perhaps add more functionality to keyword-spec.js, a function that will take a bunch of specs and combine them. Then you could test that function directly, and the code currently in this PR gets even simpler.

Yes, this would be great to have. That can definitely be in a separate PR of course.

smhg · 2021-03-28T06:08:10Z

@jsut please also run npm run lint during or after development (or make your editor run eslint). Please make sure that passes.

jsut · 2021-03-28T12:53:41Z

@jsut please also run npm run lint during or after development (or make your editor run eslint). Please make sure that passes.

done

jsut · 2021-04-01T01:16:56Z

@smhg do you need anything else here?

smhg · 2021-04-08T07:29:04Z

@jsut there are some minor changes in master. Could you merge them into your branch?

I'll check things in the next few days and release this.

- adding -k with no value will tell the parser not to use the default keywords - adding -k with a value will add a keyword to the list of keywords - add tests

jsut · 2021-04-08T13:03:27Z

@smhg it's rebased

smhg · 2021-04-08T13:10:54Z

@jsut thanks!
Running ./bin/xgettext-template with only -k (and no new keywords) throws an error.
Can you add a test for this and handle this scenario?

jsut · 2021-04-08T14:36:41Z

@smhg yup, maybe this afternoon.

smhg · 2021-04-17T14:34:02Z

Thank you for your ongoing efforts @jsut! I've had some time to look at the actual changes to the code.

The implementation you proposed to enable this new behavior is not great. I understand why you chose this route. It does work. It however makes to code harder to interpret. For instance: adding a property to the keywordSpec to later delete that property again feels like a hack. Also, the -k case (reset) should probably not be passed on to createKeywordSpec but 'detected' early on.
The tests you added are great.
Don't force push changes. Just add new commits and in the end everything will be squashed into one commit into master. It is not important in how many steps you create this.

Would you be open to further improve the PR? I'm happy to add comments to the code if you're open to this.
If you don't, feel free to only keep the changes to the tests and fixtures. We can definitely use those.

jsut · 2021-04-17T15:03:15Z

If you have a cleaner way to implement it @smhg let me know. I just tried to make it work despite it seeming like a pretty pointless thing to implement since running the thing with only a -k will never result in any output other than an empty po file anyway.

I dug into how yargs works to try to find a cleaner way to handle this, but it seemed like a dead end.

- has 2 keys, noDefaults and spec. spec is the old return value, noDefault is a boolean that indicates if they bare keyword was present indicating that the default keywords should not be applied

jsut · 2021-04-17T18:22:49Z

@smhg this seems cleaner, not loving all the spec.spec, particularly in the tests, but yeah...

smhg · 2021-04-18T12:07:29Z

@jsut thank you for your efforts. I don't think this is moving in the right direction. I'd love to offer more timely feedback, but I have to acknowledge I don't have the time for that kind of followup currently.

In general:

xgettext handles --keyword and --keyword="" as ways to disable the defaults. Currently the first one would work, but only in the right order (when another parameter follows). That's a hard problem as far as I can see, but we can't move ahead that way. I think an acceptable workaround would be to only support --keyword="" for now.
The way you implement the new feature is rather strange to me. I think it is better to 'filter' out a possible reset early on and not pass that to createKeywordSpec. So it does not need to handle that and so there is no need to return a wrapper object. I might be wrong about this.

smhg · 2021-04-18T12:31:52Z

I've added an alternative solution to the v5 branch based on your tests.
This does not handle the -k or --keyword (boolean) use however. That would still need to be added for a v5 release.

jsut · 2021-04-18T14:09:57Z

Given your branched changes, i'm not sure if i should be doing anything else here or not.

* xgettext handles `--keyword` and `--keyword=""` as ways to disable the defaults. Currently the first one would work, but **only in the right order** (when another parameter follows). That's a hard problem as far as I can see, but we can't move ahead that way. I think an acceptable workaround would be to only support `--keyword=""` for now.

I'm not sure if you're talking about gnu xgettext here, or this package. In any case, in this branch, both of them support -k/--keyword, in any position in the parameter list on the CLI, with the one caveat that in xgettext-template, if you do -k , or --keyword then yargs will slurp up the filename as the 'keyword'. This is a yargs problem, and i think it's actually impossible to fix without a significant change to yargs, or some serious massaging of what you pass into yargs. Neither of which seem reasonable to me. What does seem reasonable though, is documenting this shortcoming of -k/--keyword. It all goes back to my original complaint about the space after -k. Maybe yargs does handle this, but i'm not that familiar with it and my dive into the docs was not fruitful.

* The way you implement the new feature is rather strange to me. I think it is better to 'filter' out a possible reset early on and _not_ pass that to `createKeywordSpec`. So it does not need to handle that and so there is no need to return a wrapper object. I might be wrong about this.

This seems like a reasonable improvement to me. Do you think that the code that does this filtering should live in index.js, or should keyword-spec.js be expanded to have more exports, and one of them is the function that does this?

smhg requested changes Mar 27, 2021

View reviewed changes

jsut force-pushed the append-keywords branch from 8a23c50 to 2e6dff7 Compare March 27, 2021 17:11

jsut force-pushed the append-keywords branch from 2e6dff7 to dd1c0f1 Compare March 28, 2021 12:50

make -k work more like xgettext

dee479b

- adding -k with no value will tell the parser not to use the default keywords - adding -k with a value will add a keyword to the list of keywords - add tests

jsut force-pushed the append-keywords branch from dd1c0f1 to dee479b Compare April 8, 2021 12:57

jsut force-pushed the append-keywords branch from 7221cc5 to bd3f976 Compare April 8, 2021 22:24

Handle bare -k

eef4d56

jsut force-pushed the append-keywords branch from bd3f976 to eef4d56 Compare April 8, 2021 22:32

jsut added 2 commits April 17, 2021 14:09

make keyword spec return an deeper object

1c1e628

- has 2 keys, noDefaults and spec. spec is the old return value, noDefault is a boolean that indicates if they bare keyword was present indicating that the default keywords should not be applied

Add a couple more keyword-spec tests.

d6a1729

smhg added a commit that referenced this pull request Apr 18, 2021

add failing tests from #82

d0106d6

jsut closed this Mar 3, 2023

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

make -k other_keyword append to the defaults #82

make -k other_keyword append to the defaults #82

jsut commented Mar 26, 2021

smhg left a comment

smhg Mar 27, 2021

smhg Mar 27, 2021

smhg Mar 27, 2021

jsut Mar 27, 2021

smhg Mar 27, 2021

jsut Mar 27, 2021

jsut commented Mar 27, 2021

smhg commented Mar 27, 2021 •

edited

Loading

smhg commented Mar 28, 2021 •

edited

Loading

jsut commented Mar 28, 2021

jsut commented Apr 1, 2021

smhg commented Apr 8, 2021 •

edited

Loading

jsut commented Apr 8, 2021

smhg commented Apr 8, 2021

jsut commented Apr 8, 2021

smhg commented Apr 17, 2021

jsut commented Apr 17, 2021

jsut commented Apr 17, 2021

smhg commented Apr 18, 2021

smhg commented Apr 18, 2021

jsut commented Apr 18, 2021 •

edited

Loading

make -k other_keyword append to the defaults #82

make -k other_keyword append to the defaults #82

Conversation

jsut commented Mar 26, 2021

smhg left a comment

Choose a reason for hiding this comment

smhg Mar 27, 2021

Choose a reason for hiding this comment

smhg Mar 27, 2021

Choose a reason for hiding this comment

smhg Mar 27, 2021

Choose a reason for hiding this comment

jsut Mar 27, 2021

Choose a reason for hiding this comment

smhg Mar 27, 2021

Choose a reason for hiding this comment

jsut Mar 27, 2021

Choose a reason for hiding this comment

jsut commented Mar 27, 2021

smhg commented Mar 27, 2021 • edited Loading

smhg commented Mar 28, 2021 • edited Loading

jsut commented Mar 28, 2021

jsut commented Apr 1, 2021

smhg commented Apr 8, 2021 • edited Loading

jsut commented Apr 8, 2021

smhg commented Apr 8, 2021

jsut commented Apr 8, 2021

smhg commented Apr 17, 2021

jsut commented Apr 17, 2021

jsut commented Apr 17, 2021

smhg commented Apr 18, 2021

smhg commented Apr 18, 2021

jsut commented Apr 18, 2021 • edited Loading

smhg commented Mar 27, 2021 •

edited

Loading

smhg commented Mar 28, 2021 •

edited

Loading

smhg commented Apr 8, 2021 •

edited

Loading

jsut commented Apr 18, 2021 •

edited

Loading