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

Change the grunt config for addtextdomain to override all text domains by default #28

Merged
merged 1 commit into from Jun 27, 2017

Conversation

2 participants
@nikolov-tmw
Contributor

nikolov-tmw commented Jun 27, 2017

Related issue - #27

Changed the grunt config for addtextdomain to override all text domains by default, instead of just appending to all i18n functions.

Changed the grunt config for addtextdomain to override all text domai…
…ns by default, instead of just appending to all i18n functions
files: {
src: [ '*.php', '**/*.php', '!node_modules/**', '!php-tests/**', '!bin/**' ]
}
update_all_domains: {

This comment has been minimized.

@danielbachhuber

danielbachhuber Jun 27, 2017

Member

What's the significance of changing this from target to update_all_domains? Will it have an impact on execution?

@danielbachhuber

danielbachhuber Jun 27, 2017

Member

What's the significance of changing this from target to update_all_domains? Will it have an impact on execution?

This comment has been minimized.

@nikolov-tmw

nikolov-tmw Jun 27, 2017

Contributor

Given the following sample code, I'll list the results depending on each config.

$label = __( 'Hello World', '{{plugin_slug}}' );
$label = __( 'Hello World', 'other-plugin-slug' );
$label = __( 'Hello World' );

With addtextdomain.target:

$label = __( 'Hello World', '{{plugin_slug}}' );
$label = __( 'Hello World', 'other-plugin-slug', '{{plugin_slug}}' );
$label = __( 'Hello World', '{{plugin_slug}}' );

With addtextdomain.update_all_domains.updateDomains = true:

$label = __( 'Hello World', '{{plugin_slug}}' );
$label = __( 'Hello World', '{{plugin_slug}}' );
$label = __( 'Hello World', '{{plugin_slug}}' );

And with addtextdomain.update_all_domains.updateDomains = '{{plugin_slug}}':

$label = __( 'Hello World', '{{plugin_slug}}' );
$label = __( 'Hello World', 'other-plugin-slug', '{{plugin_slug}}' );
$label = __( 'Hello World', '{{plugin_slug}}' );

I thought I saw different results when I was testing earlier.

So it seems that the only issue that exists is that if you happened to copy code that uses a different textdomain, your domain will get appended to the i18n function(which is definitely not correct).

Not sure if the recommended should be forcing all textdomains to be the same by default. Maybe this change is not really necessary?

@nikolov-tmw

nikolov-tmw Jun 27, 2017

Contributor

Given the following sample code, I'll list the results depending on each config.

$label = __( 'Hello World', '{{plugin_slug}}' );
$label = __( 'Hello World', 'other-plugin-slug' );
$label = __( 'Hello World' );

With addtextdomain.target:

$label = __( 'Hello World', '{{plugin_slug}}' );
$label = __( 'Hello World', 'other-plugin-slug', '{{plugin_slug}}' );
$label = __( 'Hello World', '{{plugin_slug}}' );

With addtextdomain.update_all_domains.updateDomains = true:

$label = __( 'Hello World', '{{plugin_slug}}' );
$label = __( 'Hello World', '{{plugin_slug}}' );
$label = __( 'Hello World', '{{plugin_slug}}' );

And with addtextdomain.update_all_domains.updateDomains = '{{plugin_slug}}':

$label = __( 'Hello World', '{{plugin_slug}}' );
$label = __( 'Hello World', 'other-plugin-slug', '{{plugin_slug}}' );
$label = __( 'Hello World', '{{plugin_slug}}' );

I thought I saw different results when I was testing earlier.

So it seems that the only issue that exists is that if you happened to copy code that uses a different textdomain, your domain will get appended to the i18n function(which is definitely not correct).

Not sure if the recommended should be forcing all textdomains to be the same by default. Maybe this change is not really necessary?

This comment has been minimized.

@danielbachhuber

danielbachhuber Jun 27, 2017

Member

Not sure if the recommended should be forcing all textdomains to be the same by default. Maybe this change is not really necessary?

It seems like a reasonable assumption to me that all textdomains are the same for a given project. I'll wait for the go-ahead from you though.

@danielbachhuber

danielbachhuber Jun 27, 2017

Member

Not sure if the recommended should be forcing all textdomains to be the same by default. Maybe this change is not really necessary?

It seems like a reasonable assumption to me that all textdomains are the same for a given project. I'll wait for the go-ahead from you though.

This comment has been minimized.

@nikolov-tmw

nikolov-tmw Jun 27, 2017

Contributor

It seems like a reasonable assumption to me that all textdomains are the same for a given project.

There are some cases where textdomains can be different(like textdomains for the back-end and textdomains for the front-end) - I think WooCommerce does it this way. However, since this is meant to be a boilerplate it should be fine(as in it shouldn't do everything out of the box).

If you think this change is reasonable, go ahead and merge it 😃

@nikolov-tmw

nikolov-tmw Jun 27, 2017

Contributor

It seems like a reasonable assumption to me that all textdomains are the same for a given project.

There are some cases where textdomains can be different(like textdomains for the back-end and textdomains for the front-end) - I think WooCommerce does it this way. However, since this is meant to be a boilerplate it should be fine(as in it shouldn't do everything out of the box).

If you think this change is reasonable, go ahead and merge it 😃

@danielbachhuber danielbachhuber changed the title from Tweaked the grunt config for addtextdomain to Change the grunt config for addtextdomain to override all text domains by default Jun 27, 2017

@danielbachhuber danielbachhuber merged commit c655640 into wp-cli:master Jun 27, 2017

1 check passed

continuous-integration/travis-ci/pr The Travis CI build passed
Details
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment