Skip to content

Add ability to merge with existing POT file#31

Merged
schlessera merged 8 commits intomasterfrom
29-merge
Jul 4, 2018
Merged

Add ability to merge with existing POT file#31
schlessera merged 8 commits intomasterfrom
29-merge

Conversation

@swissspidy
Copy link
Copy Markdown
Member

@swissspidy 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
Copy link
Copy Markdown
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?

@swissspidy
Copy link
Copy Markdown
Member Author

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 a team June 19, 2018 14:21
@swissspidy swissspidy requested a review from schlessera July 3, 2018 07:59
*
* [--merge[=<file>]]
* : Existing POT file file whose content should be merged with the extracted strings.
* By default, the following files and folders are ignored: node_modules, .git, .svn, .CVS, .hg, vendor.
Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Merging seems to have messed up the docblocks. --merge & --exclude have the wrong explanations.

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This will also need a refresh of the README.md once fixed.

Copy link
Copy Markdown
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thanks, fixed now!

@swissspidy swissspidy requested a review from schlessera July 4, 2018 07:42
@schlessera schlessera added the command:i18n-make-pot Related to 'i18n make-pot' command label 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
@schlessera schlessera deleted the 29-merge branch July 4, 2018 07:47
@schlessera
Copy link
Copy Markdown
Member

Awesome, @swissspidy, merged another one!

schlessera added a commit that referenced this pull request Jan 6, 2022
Add ability to merge with existing POT file
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

command:i18n-make-pot Related to 'i18n make-pot' command

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants