Add more i18n sniffs #567
Add more i18n sniffs #567
Conversation
|
This looks great, thanks @deBhal! In regard to the plurals and placeholders in the wrong order, I don't think that we really need to worry about that. As you say, it is a bug in the plugin itself. Though I suppose we could check if the order is the same in both of the strings, and report in error if it isn't. In some cases it would be undetectable though, if the user was just using |
|
Thanks for the quick feedback! I've added another new sniff for the singular problem described in https://codex.wordpress.org/I18n_for_WordPress_Developers#Plurals I've also added a warning for mismatched placeholders, which will catch at least some of the cases we talked about above. I'm adding these to this PR rather than opening separate ones because the way the tests work would force annoying re-edits if they came from separate PRs. Quick question: I've used |
| $token = $tokens[ $stack_ptr ]; | ||
| static $only_once = true; | ||
| if ( $only_once ) { | ||
| // echo( '$tokens = ' . var_export( $tokens, true ) . PHP_EOL ); //DEBUG |
There was a problem hiding this comment.
The debug code here (and below on line 80) can be removed. 😃
(And just FYI, if you have never used an IDE with support for xdebug built in, you are truly missing out! 😃)
Thanks for this, I didn't realize that (or maybe had just forgotten).
I'm pretty sure that this is just a leftover from when the code called one of several different methods in that spot, but it has been refactored to just call that one now. So there is really no reason for |
| @@ -179,6 +183,14 @@ public function process( PHP_CodeSniffer_File $phpcs_file, $stack_ptr ) { | |||
| } | |||
| call_user_func( array( $this, 'check_argument_tokens' ), $phpcs_file, $argument_assertion_context ); | |||
| } | |||
|
|
|||
| // for _n*() calls, compare the singular and plural strings | |||
There was a problem hiding this comment.
Capital to start comments, and a full stop to end please.
|
Thanks, @GaryJones. I've fixed that comment and several others, and squashed the feedback changes into logical commits. |
|
I've added one more test that checks for translations that are just placeholders ( I'm not expecting to add any more tests now, but I'll still respond to feedback. |
|
Looks good to me. @GaryJones, would you like to review (and then merge)? |
This PR is to add another test to the I18nSniff.php to ensure that multiple placeholders are orderable.
e.g. from the PHP: sprintf page:
__( 'The %s contains %d monkeys' );is a problem because the variables will need to be the other order in some languages, and the string should be:__( 'The %1$s contains %2$d monkeys' );The PR includes tests and a fixer.
One thing worth discussing is that the fixer could potentially get it wrong if:
The test file gives an example of the sort of string I'm talking about:
_n( 'There is %d monkey in the %s', 'In the %s there are %d monkeys', $number, 'my-slug' );My assessment here is that this is a bug in the code, and this sniff won't fix it, but it also won't make it any worse than it already was. I was tempted to not apply the fix in plural cases, but think the good outweighs the bad, so for now the fix does run on plurals.