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

Cleanup locales.php #540

Open
toolstack opened this Issue Sep 12, 2016 · 2 comments

Comments

Projects
None yet
2 participants
@toolstack
Contributor

toolstack commented Sep 12, 2016

locales.php needs some cleanup, there are unused properties and it's not well documented.

  • Add docblocks
  • Apply coding standards
  • Directly assign the locals array instead of creating temporary variables
  • Remove the unused google_code property
  • Remove the unused facebook_code property

Remove the unused preferred_sans_serif_font_family property

I'm also throwing the following item out for discussion:

  • Remove the unused wp_locale property

To be clear, when I say 'unused' for wp_locale, I mean unused by GP. It is certainly used by wordpress.org (which I don't think can be said for the other three above), however it would seem like that should be something to include in a plugin instead of the core.

@toolstack toolstack self-assigned this Sep 12, 2016

@toolstack toolstack added the has PR label Sep 12, 2016

@ocean90

This comment has been minimized.

Member

ocean90 commented Sep 20, 2016

Add docblocks

Sounds good.

Apply coding standards

Sounds good.

Directly assign the locals array instead of creating temporary variables

I don't see the need for that. The temporary variable looks cleaner and is less code.

Remove the unused google_code property

-1. GP_Locales needs to be treated as a library which is used by others, for example Jetpack.

Remove the unused facebook_code property

-1. GP_Locales needs to be treated as a library which is used by others, for example Jetpack.

Remove the unused preferred_sans_serif_font_family property

It's not unused:

if ( $locale->preferred_sans_serif_font_family ) {

Remove the unused wp_locale property

-1. GP_Locales needs to be treated as a library which is used by others, for example Jetpack, translate.wordpress.org or other WordPress related installs like http://translate.yoast.com/.

@toolstack

This comment has been minimized.

Contributor

toolstack commented Sep 20, 2016

Directly assign the locals array instead of creating temporary variables

I don't see the need for that. The temporary variable looks cleaner and is less code.

I don't think the temporary variables looks any cleaner and is more code not less. It requires a loop over them to then assign them in to the array.

Remove the unused preferred_sans_serif_font_family property

It's not unused:

if ( $locale->preferred_sans_serif_font_family ) {

I missed that usage, I'll add it back in.

GP_Locales needs to be treated as a library which is used by others, for example Jetpack, translate.wordpress.org or other WordPress related installs like http://translate.yoast.com/.

That's how the locales.php has been treated in the past but that's not to say that it was a good idea. It adds additional items that are not used by GP directly and makes maintaining it harder. Sites that use the additional information should store it separately from GP so they can maintain it.

I ran in to this when I updated GP Machine Translate to support other providers, the Google codes were out of date and I had 3 other providers to add so instead of expanding GP_Locales even further I pulled all of it out and in to the plugin.

Maintaining information inside GP_Locales which is inherently only useful to a few sites seems like the wrong way to do it now that there are over 200 (and growing) installs of the plugin.

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