Skip to content
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

Extend PotGenerator to improve plural forms output #128

Merged
merged 3 commits into from
Mar 22, 2019

Conversation

swissspidy
Copy link
Member

Fixes #121.

Note: Might need some more tests because this adds quite a lot of code from the upstream library.

@swissspidy swissspidy changed the title [WIP] Extend PotGenerator to improve plural forms output Extend PotGenerator to improve plural forms output Dec 26, 2018
@greatislander
Copy link

@swissspidy I didn't see this— I'll test it with our project and see if it resolves the issue. Thanks!

Copy link

@greatislander greatislander left a comment

Choose a reason for hiding this comment

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

This resolves #121 in my testing.

src/PotGenerator.php Outdated Show resolved Hide resolved
}
}

if ( $translation->hasExtractedComments() ) {
Copy link
Member

Choose a reason for hiding this comment

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

This is not really needed, as you'll get an empty array back if not. Iterating over an empty array just does nothing anyway.

src/PotGenerator.php Outdated Show resolved Hide resolved

foreach ( $translations as $translation ) {
/** @var \Gettext\Translation $translation */
if ( $translation->hasComments() ) {
Copy link
Member

Choose a reason for hiding this comment

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

This is not really needed, as you'll get an empty array back if not. Iterating over an empty array just does nothing anyway.

if ( ! empty( static::$comments_before_headers ) ) {
$result = implode( "\n", static::$comments_before_headers ) . "\n";
$plural_form = $translations->getPluralForms();
$plural_size = is_array( $plural_form ) ? ( $plural_form[0] - 1 ) : 1;
Copy link
Member

Choose a reason for hiding this comment

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

This needs a comment to explain what happens and especially what $plural_form[0] - 1 means.

@schlessera
Copy link
Member

The code is not really pretty because there's so much static access going on in the original library, but I guess that can't be easily helped.

@dac514 dac514 mentioned this pull request Mar 13, 2019
@dac514
Copy link
Contributor

dac514 commented Mar 22, 2019

If this isn't going to be merged soon, can someone sync 121-plural-forms with master? It's ~23 commits behind, there are fixes this branch could benefit from. (For example; #125) Then I can simply keep using composer require wp-cli/i18n-command:dev-121-plural-forms

@schlessera
Copy link
Member

@swissspidy I'm fine with merging this now. Do you feel good about this as well?

Regarding the tests, I would assume we'll notice when things go wrong because of the Behat tests anyway, even without precise unit tests?

@swissspidy
Copy link
Member Author

Sure. We can do some housekeeping here at some point in the future I guess.

@swissspidy swissspidy merged commit e4778a0 into master Mar 22, 2019
@swissspidy swissspidy deleted the 121-plural-forms branch March 22, 2019 10:35
@schlessera
Copy link
Member

@connerbw This is merged into master now.

@schlessera schlessera added this to the 2.1.1 milestone Mar 22, 2019
dac514 added a commit to pressbooks/pb-cli that referenced this pull request Mar 22, 2019
The changes we needed have been released

See: wp-cli/i18n-command#128
dac514 added a commit to pressbooks/pb-cli that referenced this pull request Mar 22, 2019
The changes we needed have been released
See: wp-cli/i18n-command#128
schlessera pushed a commit that referenced this pull request Jan 6, 2022
Extend PotGenerator to improve plural forms output
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants