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

Add ability to merge with existing POT file #31

Merged
merged 8 commits into from Jul 4, 2018

Conversation

2 participants
@swissspidy
Collaborator

swissspidy commented May 9, 2018

Fixes #29.

Todo:

  • Make sure the headers of the resulting POT file aren't overridden by the existing one.
  • Figure out how to omit --merge and have it pick the destination file.
@schlessera

This comment has been minimized.

Show comment
Hide comment
@schlessera

schlessera Jun 5, 2018

Member

I tried to look through the code to see what is missing for:

Make sure the headers of the resulting POT file aren't overridden by the existing one

I wonder though what header data would be considered the correct one if you merge two files.

So, when using the --merge command, you can use two different paths:

  1. Merge strings from the processed folder into the existing destination pot file.
  2. Merge strings from the processed folder into an existing pot file, and create a separate destination file.

Here's what a general header would look like:

Project-Id-Version: My Plugin
Report-Msgid-Bugs-To: https://wordpress.org/support/plugin/my-plugin
Last-Translator: FULL NAME <EMAIL@ADDRESS>
Language-Team: LANGUAGE <LL@li.org>
MIME-Version: 1.0
Content-Type: text/plain; charset=UTF-8
Content-Transfer-Encoding: 8bit
POT-Creation-Date: 2018-05-02T22:06:24+00:00
PO-Revision-Date: 2018-05-02T22:06:24+00:00
X-Domain: my-plugin

Here's my first guess at how headers would be handled then, provided that we can assume we're always working on the same "Project":

  • Project-Id-Version & X-Domain should be required to be the same. Throw error if they are not.
  • Use POT-Creation-Date from destination file (existing for 1., new for 2.)
  • Override all other header settings

Does the above make sense?

Member

schlessera commented Jun 5, 2018

I tried to look through the code to see what is missing for:

Make sure the headers of the resulting POT file aren't overridden by the existing one

I wonder though what header data would be considered the correct one if you merge two files.

So, when using the --merge command, you can use two different paths:

  1. Merge strings from the processed folder into the existing destination pot file.
  2. Merge strings from the processed folder into an existing pot file, and create a separate destination file.

Here's what a general header would look like:

Project-Id-Version: My Plugin
Report-Msgid-Bugs-To: https://wordpress.org/support/plugin/my-plugin
Last-Translator: FULL NAME <EMAIL@ADDRESS>
Language-Team: LANGUAGE <LL@li.org>
MIME-Version: 1.0
Content-Type: text/plain; charset=UTF-8
Content-Transfer-Encoding: 8bit
POT-Creation-Date: 2018-05-02T22:06:24+00:00
PO-Revision-Date: 2018-05-02T22:06:24+00:00
X-Domain: my-plugin

Here's my first guess at how headers would be handled then, provided that we can assume we're always working on the same "Project":

  • Project-Id-Version & X-Domain should be required to be the same. Throw error if they are not.
  • Use POT-Creation-Date from destination file (existing for 1., new for 2.)
  • Override all other header settings

Does the above make sense?

@swissspidy

This comment has been minimized.

Show comment
Hide comment
@swissspidy

swissspidy Jun 19, 2018

Collaborator

I wonder though what header data would be considered the correct one if you merge two files.

That probably depends on what you want to do with the two files.

If you just want a simple POT file that can be imported into GlotPress, I don't think GlotPress checks the headers at all.

I think the main use case for merging strings would be the one I mentioned in #29: generating a POT file from JS strings and then extracting strings from PHP afterwards.

Project-Id-Version & X-Domain should be required to be the same. Throw error if they are not.

I'm not sure X-Domain is always present. Also, I'm not sure about the format of Project-Id-Version used by different software.

I'd suggest not adding such a requirement for now (or max. a warning) and see how that goes.

Use POT-Creation-Date from destination file (existing for 1., new for 2.)

The date should be the current date in both cases, I think. It's a new POT file with new strings after all.

Does the above make sense?

I think so! :-)

Right now, the command creates a new POT file and first adds the strings from the existing POT file to it. Now, I just need to add some tests to check for the POT headers.

After that, we can iterate from there and perhaps add more strict checks in separate PRs etc.

Collaborator

swissspidy commented Jun 19, 2018

I wonder though what header data would be considered the correct one if you merge two files.

That probably depends on what you want to do with the two files.

If you just want a simple POT file that can be imported into GlotPress, I don't think GlotPress checks the headers at all.

I think the main use case for merging strings would be the one I mentioned in #29: generating a POT file from JS strings and then extracting strings from PHP afterwards.

Project-Id-Version & X-Domain should be required to be the same. Throw error if they are not.

I'm not sure X-Domain is always present. Also, I'm not sure about the format of Project-Id-Version used by different software.

I'd suggest not adding such a requirement for now (or max. a warning) and see how that goes.

Use POT-Creation-Date from destination file (existing for 1., new for 2.)

The date should be the current date in both cases, I think. It's a new POT file with new strings after all.

Does the above make sense?

I think so! :-)

Right now, the command creates a new POT file and first adds the strings from the existing POT file to it. Now, I just need to add some tests to check for the POT headers.

After that, we can iterate from there and perhaps add more strict checks in separate PRs etc.

@swissspidy swissspidy requested a review from wp-cli/committers Jun 19, 2018

swissspidy added some commits Jul 3, 2018

@swissspidy swissspidy requested a review from schlessera Jul 3, 2018

Show outdated Hide outdated src/MakePotCommand.php

@swissspidy swissspidy requested a review from schlessera Jul 4, 2018

@schlessera schlessera added this to the 0.1.0 milestone Jul 4, 2018

@schlessera schlessera merged commit 22cd97a into master Jul 4, 2018

1 check passed

continuous-integration/travis-ci/pr The Travis CI build passed
Details

@schlessera schlessera deleted the 29-merge branch Jul 4, 2018

@schlessera

This comment has been minimized.

Show comment
Hide comment
@schlessera

schlessera Jul 4, 2018

Member

Awesome, @swissspidy, merged another one!

Member

schlessera commented Jul 4, 2018

Awesome, @swissspidy, merged another one!

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment