Support for merging .pot file into .po file

[cedaro]

Support for merging .pot file into .po file

[bradyvercher]

Hi @atimmer, the makepo task that's still in development will only create a PO file if one doesn't already exist. I added a note that msgmerge would be useful, but I'm not currently aware of any Node-based alternatives (nothing in grunt-wp-i18n currently depends on gettext).

I imagine progressively enhancing the existing functionality when the gettext utilities are available can't hurt and can be replaced if an alternative ever becomes available.

With that in mind, I can see this being useful in a couple of places:

1. A new option in the makepot task would allow all the PO files to be updated any time the POT file is generated.
2. The makepo task currently only works for creating a new PO file for a single locale. It might be possible to update that task to run msgmerge on all existing PO files when it's called or the single locale file when it's specified.

We can probably go ahead with 2 for now, while keeping 1 in mind for the future. Does that sounds like it'd solve your use case and something you'd be interested in submitting a PR for?

[atimmer]

I made a pull request that is a very small task wrapper around msgmerge.

We can solve both cases using this task. Do you think it is a good idea to have it in a separate task or should it somehow be a 'private' function?

[bradyvercher]

Nice work, @atimmer! I'm not too keen on exposing a new task, though, so I'm thinking the functionality could be wrapped up in a module in the /tasks/lib directory and a new option can be added to the makepot task to enable it (something like updatePoFiles: true), but I'm open to suggestions.

[atimmer]

I updated the pull request with the suggestions. updatePoFiles is a great idea, currently it is off by default. Tell me if you want it on by default. msgMerge is moved to /tasks/lib.

When makepo is implemented it can make use of msgMerge.

If this is the correct direction I will add some documentation to the pull request.

[bradyvercher]

This looks great. Off by default is good since that option requires an external dependency.

When you add the docs, this section could use a few spaces to match the WP JS coding standards. Let me know when you get everything wrapped up and I'll do some testing and merge this.

Thanks a ton!

[atimmer]

The pull request should be done now. I've added some documentation and support for themes, because the PO files format is different for them.

[bradyvercher]

I left some comments on a couple of the commits for issues I noticed after pulling this down and giving it a quick run through. Once those are addressed, I think this should be good to go.

[atimmer]

They should all be fixed now, I actually got the backup files as well but my editor doesn't show them.

[bradyvercher]

Looks good! Once last thing, can you squash these commits? I'll merge it after that.

[atimmer]

Squashed.

[bradyvercher]

Merged. I'll run a few more tests and try to push out an update soon. Thanks again!

[GaryJones]

I'm seeing the tests fail with this

Running "makepot:msg_merge_merging" (makepot) task
>> POT file saved to tmp/msg-merge/msg-merge.pot
>> msgmerge error:Error: Error: not found: msgmerge
>> msgmerge error:Error: Error: not found: msgmerge

Maybe I haven't got msgmerge available as a tool on this machine, but the tests should check for that and skip those with a suitable message.

  Testing makepot_test.js..........F.F.
>> makepot - msg_merge_merging
>> Message: "Colors" string should exist
>> Error: undefined == true
>> at Object.exports.makepot.msg_merge_merging (test/makepot_test.js:173:8)
>> at Object.exports.makepot.setUp (test/makepot_test.js:28:3)

>> makepot - msg_merge_merging
>> Message: "Colors" string should exist
>> Error: undefined == true
>> at Object.exports.makepot.msg_merge_merging (test/makepot_test.js:174:8)
>> at Object.exports.makepot.setUp (test/makepot_test.js:28:3)

>> makepot - msg_merge_merging
>> Error: Expected 4 assertions, 2 ran
>> at processImmediate [as _immediateCallback] (timers.js:358:17)

>> makepot - msg_merge_merging
>> TypeError: Cannot read property 'comments' of undefined
>> at Object.exports.makepot.msg_merge_merging (test/makepot_test.js:176:47)
>> at Object.exports.makepot.setUp (test/makepot_test.js:28:3)

>> makepot - msg_merge_theme
>> Message: "Analog string should not exist
>> Error: false == true
>> at Object.exports.makepot.msg_merge_theme (test/makepot_test.js:205:8)
>> at Object.exports.makepot.setUp (test/makepot_test.js:28:3)

>> makepot - msg_merge_theme
>> Message: "Digital" string should exist
>> Error: undefined == true
>> at Object.exports.makepot.msg_merge_theme (test/makepot_test.js:206:8)
>> at Object.exports.makepot.setUp (test/makepot_test.js:28:3)

Warning: 6/59 assertions failed

(Also appears to be missing a double quote in one of the error message strings).

[bradyvercher]

I took a look into excluding those tests if msgmerge isn't available, but there doesn't appear to be a good way to detect whether or not an executable exists or to skip tests.

The one thing I can think of is to split the tests and add a flag to skip the ones that depend on msgmerge. Anyone have a better idea?

[atimmer]

To check if gettext is available I thought of two approaches:

• Add a isGettextAvailable function to the util file that calls gettext --version and catches the error gracefully.
• This: https://github.com/arturadib/shelljs and then call which( 'msgmerge' ) to check for the command.

[GaryJones]

Relevant (but old): caolan/nodeunit#12

[shivapoudel]

Very easy, just be smart ;)

You can use shelljs for checking if msgmerge is found in system PATH or not.
https://github.com/axisthemes/grunt-potomo/blob/master/tasks/potomo.js#L20-L23

The shelljs module which() is prepared to deal with command in the system's PATH:
https://github.com/arturadib/shelljs/blob/master/src/which.js#L33-L82

As I said, just be smart ;)