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

Introduce $piglatin_domains and piglatin_domains filter #6

Closed
wants to merge 2 commits into from

Conversation

rmccue
Copy link

@rmccue rmccue commented Apr 7, 2014

Fixes #4. Adds both the $piglatin_domains global variable and a piglatin_domains filter.

By default, allows all domains unless either of the above is used, in which case, only uses domains set in that variable. To use, add something like this to your wp-config.php:

$piglatin_domains = [
    'myplugin' => true,
];

@rmccue
Copy link
Author

rmccue commented Apr 12, 2014

@nb Spacing fixed up.

@nb
Copy link
Owner

nb commented Apr 12, 2014

Thank you for the contribution, @rmccue!

I am still thinking what would be the best way to enable/disable the plugin in different situations. See, for example #3 for another example of this problem.

Could you explain in what use case did you need to filter by domain?

Few notes about the code:

  • It would be great if you don’t have to add the check_domain condition to each gettext function.
  • Can we find more elegant method than a global?

@rmccue
Copy link
Author

rmccue commented Apr 12, 2014

Thank you for the contribution, @rmccue!

Thanks for the fantastic plugin! :)

I am still thinking what would be the best way to enable/disable the plugin in different situations. See, for example #3 for another example of this problem.

Yeah, I'm not too concerned about disabling on the fly. For me, it's a local development thing, so toggling one line in wp-config.php isn't a big issue.

Could you explain in what use case did you need to filter by domain?

Sure! When developing plugins, it helps to be able to visually check that you've included the correct domain in your translations. Pig Latin is great for finding untranslated strings, but not for checking that. It's really easy (and really common) to accidentally leave the domain out.

I also use Pig Latin to help visually check translation on projects with more than one text domain; for example, Happytables's dashboard has the domain dashboard, whereas the themes themselves have the domain frontend. Checking that these domains are correct is a hard job, since the code isn't always as separated as it should be.

  • It would be great if you don’t have to add the check_domain condition to each gettext function.

I agree. The alternative is to pass it into translation2pig, which feels too deep in the stack to me. We could also refactor, but each function takes different parameters, so we can't necessarily do so.

  • Can we find more elegant method than a global?

Alas, a global is the only way to get it to work with adding things into wp-config.php. Filters can't be added into the config easily. We could do it in a UI/option, but that's a lot of work for a fairly minor feature. :)

@nb
Copy link
Owner

nb commented Apr 13, 2014

Pig Latin is great for finding untranslated strings, but not for checking that. It's really easy (and really common) to accidentally leave the domain out.

I agree. Though I not entirely agree that it should do that. In general add-textdomain.php is pretty good at this.

In your case, since one file has more than one domain using this tool might be tricky.

I don’t think we should include this change, because the majority of the users of the plugin won’t benefit from it.

However, I think it would be great if we add a filter, which will let you selectively disable the plugin – either based on domain, on an UI option, on the current environment (dev/translate/production), or on anything you’d like. Pull requests are, again, welcome :-)

@mnelson4
Copy link

FYI our team is often using the piglatin plugin to verify that we're internationalizing our plugins. But it of course doesn't catch problems when we accidentally leave off our textdomain from the i18n method calls (eg, when we do _e( 'some phrase' ); instead of _e( 'some_phrase', 'my_text_domain');).
So just an upvote for this pull request

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Add text domain option
3 participants