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

666: Add CLDR support #784

Open
wants to merge 31 commits into
base: develop
from
Open

666: Add CLDR support #784

wants to merge 31 commits into from

Conversation

@toolstack
Copy link
Contributor

toolstack commented Jul 7, 2017

Resolves #666.

@toolstack
Copy link
Contributor Author

toolstack commented Jul 7, 2017

A couple of notes on the current state of this:

  • I have only added CLDR plurals support to the android format, the Apple strings format remains to be done.
  • The test cases haven't been updated to do importing of CLDR plurals yet and the export test case is a bit of a fudge ;)
  • There's probably a few more docblocks to be written.
@toolstack toolstack requested a review from ocean90 Jul 7, 2017
@toolstack toolstack force-pushed the 666-cldr-support branch from 451a901 to f3492ba Jul 7, 2017
Copy link
Member

yoavf left a comment

Really happy to see this PR. Left a few comments on a few very minor issues spotted while taking a quick look

/*
* Get the CLDR Data.
*
* By defualt it is read from the plurals.json file in the current directory, you can download

This comment has been minimized.

Copy link
@yoavf

yoavf Jul 10, 2017

Member

typo defualt

// Create a working locales object.
$locales = new GP_Locales;

// Run through the locales and see if we can find a mathcing CLDR locale.

This comment has been minimized.

Copy link
@yoavf

yoavf Jul 10, 2017

Member

Typo in mathcing

@@ -24,11 +27,22 @@ function test_export() {
$entries_for_export = array();

foreach( $this->entries as $sample ) {
list( $context, $original, $translation ) = $sample;
list( $context, $original, $translation, $comment ) = $sample;

This comment has been minimized.

Copy link
@yoavf

yoavf Jul 10, 2017

Member

Is the addition of the comment intentional in this patch?

This comment has been minimized.

Copy link
@toolstack

toolstack Jul 10, 2017

Author Contributor

Yes, but only because it fixes an existing bug in the test case.

@toolstack toolstack added this to the 3.0 milestone Jul 11, 2017
@toolstack toolstack force-pushed the 666-cldr-support branch from 16af3e9 to df61191 Aug 31, 2017
@toolstack toolstack force-pushed the 666-cldr-support branch from 6cfe241 to 8890fcf Oct 2, 2017
*
* @return string HTML markup for a select element.
*/
function gp_plurals_dropdown( $name_and_id, $selected_plural_type = null, $attrs = array() ) {

This comment has been minimized.

Copy link
@ocean90

ocean90 Oct 3, 2017

Member

Let's name this gp_plural_types_dropdown().

This comment has been minimized.

Copy link
@toolstack

toolstack Oct 3, 2017

Author Contributor

👍

@@ -12,6 +12,9 @@
<dt><label for="project[description]"><?php _e( 'Description', 'glotpress' ); ?></label> <span class="ternary"><?php _e( 'can include HTML', 'glotpress' ); ?></span></dt>
<dd><textarea name="project[description]" rows="4" cols="40" id="project[description]"><?php echo esc_html( $project->description ); ?></textarea></dd>

<dt><label for="project[plurals_type]"><?php _e( 'Plurals Type', 'glotpress' ); ?></label></dt>
<dd><?php echo gp_plurals_dropdown( 'project[plurals_type]', $project->plurals_type, array() ); // WPCS: XSS ok. ?></dd>

This comment has been minimized.

Copy link
@ocean90

ocean90 Oct 3, 2017

Member

gp_plurals_dropdown() should be added to customAutoEscapedFunctions in phpcs.xml.

This comment has been minimized.

Copy link
@toolstack

toolstack Oct 3, 2017

Author Contributor

👍

} // End foreach().

// Add some space between locales for easier reading.
echo "\n"; // WPCS: XSS ok.

This comment has been minimized.

Copy link
@ocean90

ocean90 Oct 3, 2017

Member

Can we ignore the variants part in this PR for now?

This comment has been minimized.

Copy link
@toolstack

toolstack Oct 3, 2017

Author Contributor

Not easily without breaking one or both of the patches :)

They both do a lot of work on the locales file so separating them is hard.

}
}
}
} // End foreach().

This comment has been minimized.

Copy link
@ocean90

ocean90 Oct 3, 2017

Member

These are no longer required and can be removed.

This comment has been minimized.

Copy link
@toolstack

toolstack Oct 3, 2017

Author Contributor

👍

$root_var_name = str_replace( '-', '_', $locale->variant_root );

// Output the first line to 'create' the GP_Locale object for this locale.
echo "\t\t\${$var_name} = new GP_Locale();\n"; // WPCS: XSS ok.

This comment has been minimized.

Copy link
@ocean90

ocean90 Oct 3, 2017

Member

Remove all // WPCS: XSS ok. comments and add the bin directory as a exclude-pattern to phpcs.xml.

This comment has been minimized.

Copy link
@toolstack

toolstack Oct 3, 2017

Author Contributor

👍

}
}
}
} // End foreach().

This comment has been minimized.

Copy link
@ocean90

ocean90 Oct 3, 2017

Member

These are no longer required and can be removed.

This comment has been minimized.

Copy link
@toolstack

toolstack Oct 3, 2017

Author Contributor

👍

@toolstack toolstack force-pushed the 666-cldr-support branch 2 times, most recently from 2a41a26 to 5a3ec10 Oct 9, 2017
@toolstack toolstack force-pushed the 666-cldr-support branch 4 times, most recently from 44f8127 to e171d3a Oct 17, 2017
@toolstack toolstack force-pushed the 666-cldr-support branch from 4094482 to e1860d8 Nov 23, 2017
@@ -12,6 +12,9 @@
<dt><label for="project[description]"><?php _e( 'Description', 'glotpress' ); ?></label> <span class="ternary"><?php _e( 'can include HTML', 'glotpress' ); ?></span></dt>
<dd><textarea name="project[description]" rows="4" cols="40" id="project[description]"><?php echo esc_html( $project->description ); ?></textarea></dd>

<dt><label for="project[plurals_type]"><?php _e( 'Plurals Type', 'glotpress' ); ?></label></dt>
<dd><?php echo gp_plural_types_dropdown( 'project[plurals_type]', $project->plurals_type, array() ); // WPCS: XSS ok. ?></dd>

This comment has been minimized.

Copy link
@ocean90

ocean90 Apr 8, 2018

Member

The // WPCS: XSS ok seems to be obsolete.

Copy link
Member

ocean90 left a comment

The current locale alignment doesn't match our code styling. Example:

<?php
$av = new GP_Locale();
$av->english_name                         = 'Avaric';
$av->native_name                          = 'авар мацӀ';
$av->text_direction                       = 'ltr';
$av->lang_code_iso_639_1                  = 'av';
$av->lang_code_iso_639_2                  = 'ava';
$av->slug                                 = 'av';
$av->nplurals                             = '2';
$av->plural_expression                    = 'n != 1';

produces:

-----------------------------------------------------------------------------------------------------------------
FOUND 0 ERRORS AND 9 WARNINGS AFFECTING 9 LINES
-----------------------------------------------------------------------------------------------------------------
  2 | WARNING | [x] Equals sign not aligned with surrounding assignments; expected 22 spaces but found 1 space
  3 | WARNING | [x] Equals sign not aligned with surrounding assignments; expected 8 spaces but found 25 spaces
  4 | WARNING | [x] Equals sign not aligned with surrounding assignments; expected 9 spaces but found 26 spaces
  5 | WARNING | [x] Equals sign not aligned with surrounding assignments; expected 6 spaces but found 23 spaces
  6 | WARNING | [x] Equals sign not aligned with surrounding assignments; expected 1 space but found 18 spaces
  7 | WARNING | [x] Equals sign not aligned with surrounding assignments; expected 1 space but found 18 spaces
  8 | WARNING | [x] Equals sign not aligned with surrounding assignments; expected 16 spaces but found 33 spaces
  9 | WARNING | [x] Equals sign not aligned with surrounding assignments; expected 12 spaces but found 29 spaces
 10 | WARNING | [x] Equals sign not aligned with surrounding assignments; expected 3 spaces but found 20 spaces
-----------------------------------------------------------------------------------------------------------------
PHPCBF CAN FIX THE 9 MARKED SNIFF VIOLATIONS AUTOMATICALLY
-----------------------------------------------------------------------------------------------------------------

It needs to be:

$av                      = new GP_Locale();
$av->english_name        = 'Avaric';
$av->native_name         = 'авар мацӀ';
$av->text_direction      = 'ltr';
$av->lang_code_iso_639_1 = 'av';
$av->lang_code_iso_639_2 = 'ava';
$av->slug                = 'av';
$av->nplurals            = '2';
$av->plural_expression   = 'n != 1';
$af->cldr_code = 'af';
$af->cldr_nplurals = '2';
$af->cldr_plural_expressions['one'] = 'n = 1 @integer 1 @decimal 1.0, 1.00, 1.000, 1.0000';
$af->cldr_plural_expressions['other'] = ' @integer 0, 2~16, 100, 1000, 10000, 100000, 1000000, … @decimal 0.0~0.9, 1.1~1.6, 10.0, 100.0, 1000.0, 10000.0, 100000.0, 1000000.0, …';

This comment has been minimized.

Copy link
@ocean90

ocean90 Apr 8, 2018

Member

Is the leading whitespace required?

This comment has been minimized.

Copy link
@toolstack

toolstack May 17, 2018

Author Contributor

I don't think so but it's in the original JSON file from the CLDR repo so I think it's safer to leave in.

@@ -162,6 +274,11 @@ private function add_schema_info() {
$this->line('-->', 1 );
}

/**
* Cteate the .resx schema declaration. .resx file have a standard scheam declaration.

This comment has been minimized.

Copy link
@GaryJones

GaryJones Apr 9, 2018

Several typos here.

This comment has been minimized.

Copy link
@toolstack

toolstack May 17, 2018

Author Contributor

Fixed.

private function escape( $string ) {
$string = str_replace( array( '&', '<' ), array( '&amp;', '&lt;' ), $string );
return $string;
}


/**
* Cteate the .resx schema info. .resx file have a standard scheam block.

This comment has been minimized.

Copy link
@GaryJones

GaryJones Apr 9, 2018

Several typos here.

This comment has been minimized.

Copy link
@toolstack

toolstack May 17, 2018

Author Contributor

Fixed.

phpcs.xml Outdated
in the current version of phpcs (see https://github.com/squizlabs/PHP_CodeSniffer/issues/1731)
which breaks this when a single file name is passed to be checked (for example with TravisCI).
A fix has been committed and will be included in phpcs version 3.2, however this is no release

This comment has been minimized.

Copy link
@GaryJones

GaryJones Apr 9, 2018

PHPCS 3.2 was released in December 2017, and the latest release is 3.2.3, with 3.3.0 not too far away. It would be reasonable to make ^3.2 the minimum version of PHPCS required for devs to contribute to GlotPress.

This comment has been minimized.

Copy link
@toolstack

toolstack May 17, 2018

Author Contributor

This should have automatically taken care of itself, we just have to remove the extra lines now as they were only needed before 3.2 was released.

@toolstack toolstack force-pushed the 666-cldr-support branch from 2ad7e51 to db99fc2 May 17, 2018
@toolstack toolstack force-pushed the 666-cldr-support branch 3 times, most recently from dcbfaf9 to 83b45fe Oct 30, 2018
@toolstack toolstack force-pushed the 666-cldr-support branch from 192f0bc to b0530f1 Jan 2, 2019
toolstack added 26 commits Jul 7, 2017
And add a minor comment to the locale generator.
Rename dropdown function and PHP CS rule additions.

Fix error.
Import not yet implemented.
Now appends the code around the locales list to make life easier.
To create no-variants version of the locales file and also write both out to files instead of the console.  Also make a backup just in case before deleting the old file.
@toolstack toolstack force-pushed the 666-cldr-support branch 2 times, most recently from 7264bcb to e7c0296 Jan 3, 2019
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Linked issues

Successfully merging this pull request may close these issues.

4 participants
You can’t perform that action at this time.