Add checking for translators comments to the I18n sniff. #742

jrfnl · 2016-12-25T11:37:41Z

Implemented based on the following specs:

Only one error should be thrown per gettext call.
A translators comment is only required when (one of) the text string(s) contains placeholders.
It's ok for there to be a translators comment if the text string(s) does not contain placeholders.
The translators comment must be directly above the gettext call.
If the comment is not on the line directly above the gettext call, there should only be whitespace lines between the comment and the line containing the gettext call.
The translators comment must be in // or /* */ format, docblocks are not allowed.
A translators comment must start with translators:
The check defaults to throwing a warning. If the extra ruleset is enabled, the error level will change to error.

Fixes #423

I've added a sniff property $check_translator_comments to enable us to unit test the other checks within the I18n sniff without getting bogged down by errors thrown by the translators comment check.
This is a public property, but is only intended to be used in combination with the unit tests.
It could be used in custom rulesets too, but this shouldn't be needed as the translator comment checks can be excluded via their error codes.

Regarding the travis run failure:

The unit tests against PHPCS 2.7.0 are failing because the ruleset is erroneously taken into account when running the unit tests, which leads to PHPCS throwing an error instead of the expected warning.

This was fixed in PHPCS 2.7.1.
This only affects the unit tests however, not the functioning of the sniff itself.

Related: #733, #696 and squizlabs/PHP_CodeSniffer#1177

GaryJones · 2016-12-26T12:10:00Z

WordPress/Sniffs/WP/I18nSniff.php

+							$comment_text = trim( $tokens[ $previous_comment ]['content'] );
+
+			  		   		// If it's multi-line /* */ comment, collect all the parts.
+			  		   		if ( '*/' === substr( $comment_text, -2 ) && '/*' !== substr( $comment_text, 0, 2 ) ) {


Could this "collect all parts from a multi-line comment" be abstracted into a re-usable method in the base class?

Also, how does it handle something like:

/* * This is a multiline comment, * But it also has * at the start of some lines ;-) */

Could this "collect all parts from a multi-line comment" be abstracted into a re-usable method in the base class?

It could, but I've not seen any other sniff within the WPCS which would need it. I suggest this can be done later if/when another sniff would need something similar.
Also - the snippet only handles multi-line /* */ type comments, no other type and (currently) does not care about how the lines are added together as for this sniff, only the start of the text is interesting, so if we would want it to be more generic, it will need some tweaking.

how does it handle something like <code snippet>

It handles that fine, just tested it to be sure + added an additional unit test to that effect.

GaryJones · 2016-12-26T12:14:17Z

WordPress/Tests/WP/I18nUnitTest.1.inc

+printf(
+	/* translators: number of monkeys, location. */
+	__( 'There are %1$d monkeys in the %2$s', 'my-slug' ),
+	intval( $number ),


intval tends to be PHP 4 - (int) would be better.

GaryJones · 2016-12-26T12:16:02Z

Code generally looks good - great work.

Pinging @ramiy, as I think he does a lot with translator comments, and can offer feedback on any missed cases / false positives.

Implemented based on the following specs: * Only one error should be thrown per gettext call. * A translators comment is only required when (one of) the text string(s) contains placeholders. * It's ok for there to be a translators comment if the text string(s) does not contain placeholders. * The translators comment must be directly above the gettext call. * If the comment is not on the *line* directly above the gettext call, there should only be whitespace lines between the comment and the line containing the gettext call. * The translators comment must be in `//` or `/* */` format, docblocks are not allowed. * A translators comment must start with `translators:` * The check defaults to throwing a warning. If the `extra` ruleset is enabled, the error level will change to `error`.

jrfnl · 2016-12-29T18:36:14Z

Rebased after #733 to allow unit tests to pass.

ramiy · 2016-12-30T02:05:18Z

Looks good!!!

As for: The translators comment must be directly above the gettext call.

And the examples on the testing file:

printf(
	/* translators: number of monkeys, location. */
	__( 'There are %1$d monkeys in the %2$s', 'my-slug' ),
	(int) $number,
	esc_html( $string )
); // Ok.

/* translators: number of monkeys, location. */
printf(
	__( 'There are %1$d monkeys in the %2$s', 'my-slug' ),
	(int) $number,
	esc_html( $string )
); // Bad - comment not directly before line containing the gettext call.

Although I prefer to add comments above the gettext call, WordPress core has tons of translators comments above the printf() and sprintf() functions. We will need to create a new ticket to move all those comments. Pinging @SergeyBiryukov and @ocean90.

jrfnl · 2016-12-30T02:44:44Z

Hi @ramiy Thanks for looking this over!

As for: The translators comment must be directly above the gettext call.

I've set that quite strict as in my personal experience with PoEdit, the comments don't get picked up otherwise. Not sure how well GlotPress/makepot does in picking up those comments if they are not on the line directly above (I expect just as badly as PoEdit uses makepot underneath the hood).

If it's on one line, it should be fine, see the last unit test:

/* translators: number of monkeys, location. */
printf( __( 'There are %1$d monkeys in the %2$s', 'my-slug' ), intval( $number ), esc_html( $string ) ); // Ok - comment is directly before line containing the gettext call.

And if the gettext call is on the same line as the (s)printf(), but the parameters aren't, it should be fine as well:

/* translators: number of monkeys, location. */
printf( __(
	'There are %1$d monkeys in the %2$s', 'my-slug' ),
	intval( $number ),
	esc_html( $string )
); // Ok - comment is directly before line containing the gettext call.

jrfnl · 2016-12-30T03:01:48Z

I've just read through a number of trac tickets around this, starting from https://core.trac.wordpress.org/ticket/39116 and am left with one question: should translators: always be lowercase of should Translators: be allowed too ? (currently it's not)
IIRC PoEdit only accepts one or the other (it is a case-sensitive command line option you provide), not sure about other makepot tools.

grappler · 2016-12-30T08:09:44Z

as PoEdit uses makepot underneath the hood

I don't think so because PoEdit had to add this functionality.

Poedit did support the "TRANSLATORS:" tag mentioned in gettext's own manual, but not the lowercase variant.

I found this in one of my emails with the PoEdit Developer.

ntwb · 2016-12-30T08:13:09Z

... am left with one question: should translators: always be lowercase

Edit: ~~Yes~~ No

Edit: Found it, it wasn't capitalisation I was thinking of, I'll post a follow up email for repo watchers

ntwb · 2016-12-30T08:41:51Z

Via https://core.trac.wordpress.org/ticket/30603

Translator comments must be on a new line, and the line begin with either /* or //

GaryJones · 2016-12-30T09:34:44Z

Comments with lowercase t may trigger other warnings about starting a comment with a capital, unless we have exclusions for them in place - we currently have that check turned off in the Docs ruleset, because those exclusions don't exist at the sniff-level yet.

I'd love to allow just capitalised Translators, but clearly prior art (namely, anyone who's ever added in these comments), means the lowercase versions needs to be supported too.

ocean90 · 2016-12-30T11:17:54Z

As for: The translators comment must be directly above the gettext call.

I've set that quite strict as in my personal experience with PoEdit, the comments don't get picked up otherwise. Not sure how well GlotPress/makepot does in picking up those comments if they are not on the line directly above (I expect just as badly as PoEdit uses makepot underneath the hood).

Just a general FYI: GlotPress doesn't generate POT files from source files, that's why we have makepot.

makepot doesn't care where a translator comment is. If there is a comment it stores it and adds the comment to the next gettext call, no matter how many lines there are between the comment and the actual gettext call.

We will need to create a new ticket to move all those comments.

There is no need for that, see also https://make.wordpress.org/core/handbook/contribute/code-refactoring/.

jrfnl · 2016-12-30T12:05:06Z

@grappler

I don't think so because PoEdit had to add this functionality.

It did for the automatic WP project setup which it now offers out of the box, but before that they already supported it as long as you configured it right:
Preferences -> Extractors -> PHP ->"Command to extra translations' - if you added "--add-comments=translators:" to this, it worked, even in (very) old versions of PoEdit.
That is the bit which I know only works in a case-sensitive way.

Oh and it also corrects me regarding what they use under the hood - not makepot, but xgettext.

Includes additional unit tests.

jrfnl · 2016-12-30T12:46:10Z

Based on the feedback I've made the checking for the translators: part case-insensitive and added unit tests to that effect.
I've also added the additional case from #742 (comment) to the unit tests (second example).

makepot doesn't care where a translator comment is. If there is a comment it stores it and adds the comment to the next gettext call, no matter how many lines there are between the comment and the actual gettext call.

Based on this: should we loosen up the check for a translators comment on the line directly above the gettext call ?
I'd advocate against it for the following reasons:

We know that if it's on the line directly above it will get picked up correctly by different tools, this might not be consistently so for all tools if the comment has other code between it and the gettext call.
Similar to the late escaping philosophy, having the translators comment tightly coupled with the actual call makes it easier for other devs to see that there is one in place.

Last question as asked before in #423 (comment):

Oh and input on whether or not some form of whitelisting for a gettext call without translators comment is needed and what form it should take would be welcome too !

I can imagine we should have a whitelist comment as - if I remember correctly - if the same phrase is used several times, it only needs a translators comment once.

Opinions ?

vslavik · 2016-12-31T12:35:57Z

It did for the automatic WP project setup which it now offers out of the box, but before that they already supported it as long you configured it right:
Preferences -> Extractors -> PHP ->"Command to extra translations' - if you added "--add-comments=translators:" to this, it worked, even in (very) old versions of PoEdit.
That is the bit which I know only works in a case-sensitive way.

Oh and it also corrects me regarding what they use under the hood - not makepot, but xgettext.

Poedit developer here. That’s correct, and the limitations come from the de-facto standard GNU gettext tools, not Poedit itself: there can only be one such tag (further --add-comments override it, not amend) and it is case-sensitive. In Poedit 2, this will be user-configurable easily and on per-PO file basis to accommodate WP (and will default to “translators:” for it). I created an issue for this for public visibility into progress: vslavik/poedit#314

Based on this: should we loosen up the check for a translators comment on the line directly above the gettext call ?
I'd advocate against it for the following reasons:

Allow me to add another reason: compatibility with xgettext extraction.

jrfnl · 2016-12-31T14:46:00Z

@vslavik Thank you for your input & clarification! Very useful.

jrfnl · 2017-01-05T17:11:22Z

Anyone still wants to have a look at this ? If not, I propose we merge it tomorrow.

jrfnl added Status: Review ready Type: Enhancement labels Dec 25, 2016

jrfnl added this to the 0.11.0 milestone Dec 25, 2016

jrfnl mentioned this pull request Dec 25, 2016

Add a translator comment sniff #423

Closed

GaryJones reviewed Dec 26, 2016

View reviewed changes

GaryJones approved these changes Dec 26, 2016

View reviewed changes

jrfnl force-pushed the feature/issue-423-check-translator-comment branch from 73fd741 to ed0eb5d Compare December 29, 2016 18:28

Check for translators comment in a case-insensitive manner.

d4739bd

Includes additional unit tests.

jrfnl merged commit 7f4c535 into develop Jan 7, 2017

jrfnl deleted the feature/issue-423-check-translator-comment branch January 7, 2017 00:17

ocean90 mentioned this pull request Jan 7, 2017

Don't require a translators comment for "foo 100% bar" #776

Closed

westonruter mentioned this pull request Jan 7, 2017

Add node-sass as devDependency to fix grunt-sass wp-shortcake/shortcake#685

Merged

JDGrimes mentioned this pull request Jan 9, 2017

Add translator comments for all placeholders in gettext calls WordPoints/wordpoints#603

Closed

jrfnl mentioned this pull request Jan 23, 2017

I18n: Add some missing @since tags #819

Merged

jrfnl mentioned this pull request Mar 6, 2017

Changelog for version 0.11.0 #866

Merged

jrfnl mentioned this pull request Feb 21, 2018

Improve PHP comments extraction php-gettext/Gettext#161

Closed

GaryJones removed the Status: Review ready label Aug 21, 2018

jrfnl added the Focus: I18n label Mar 31, 2019

jrfnl mentioned this pull request Nov 29, 2020

Fixes I18NSniff for multi-line gettext calls #1962

Open

jrfnl mentioned this pull request Mar 18, 2023

WP/I18n: complete refactor, implement PHPCSUtils, modern PHP and more #2214

Merged

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Add checking for translators comments to the I18n sniff. #742

Add checking for translators comments to the I18n sniff. #742

jrfnl commented Dec 25, 2016

GaryJones Dec 26, 2016 •

edited

Loading

jrfnl Dec 26, 2016 •

edited

Loading

GaryJones Dec 26, 2016

jrfnl Dec 26, 2016

GaryJones commented Dec 26, 2016

jrfnl commented Dec 29, 2016

ramiy commented Dec 30, 2016 •

edited by jrfnl

Loading

jrfnl commented Dec 30, 2016

jrfnl commented Dec 30, 2016

grappler commented Dec 30, 2016 •

edited

Loading

ntwb commented Dec 30, 2016 •

edited

Loading

ntwb commented Dec 30, 2016

GaryJones commented Dec 30, 2016 •

edited

Loading

ocean90 commented Dec 30, 2016

jrfnl commented Dec 30, 2016 •

edited

Loading

jrfnl commented Dec 30, 2016

vslavik commented Dec 31, 2016

jrfnl commented Dec 31, 2016

jrfnl commented Jan 5, 2017

Add checking for translators comments to the I18n sniff. #742

Add checking for translators comments to the I18n sniff. #742

Conversation

jrfnl commented Dec 25, 2016

Regarding the travis run failure:

GaryJones Dec 26, 2016 • edited Loading

Choose a reason for hiding this comment

jrfnl Dec 26, 2016 • edited Loading

Choose a reason for hiding this comment

GaryJones Dec 26, 2016

Choose a reason for hiding this comment

jrfnl Dec 26, 2016

Choose a reason for hiding this comment

GaryJones commented Dec 26, 2016

jrfnl commented Dec 29, 2016

ramiy commented Dec 30, 2016 • edited by jrfnl Loading

jrfnl commented Dec 30, 2016

jrfnl commented Dec 30, 2016

grappler commented Dec 30, 2016 • edited Loading

ntwb commented Dec 30, 2016 • edited Loading

ntwb commented Dec 30, 2016

GaryJones commented Dec 30, 2016 • edited Loading

ocean90 commented Dec 30, 2016

jrfnl commented Dec 30, 2016 • edited Loading

jrfnl commented Dec 30, 2016

vslavik commented Dec 31, 2016

jrfnl commented Dec 31, 2016

jrfnl commented Jan 5, 2017

GaryJones Dec 26, 2016 •

edited

Loading

jrfnl Dec 26, 2016 •

edited

Loading

ramiy commented Dec 30, 2016 •

edited by jrfnl

Loading

grappler commented Dec 30, 2016 •

edited

Loading

ntwb commented Dec 30, 2016 •

edited

Loading

GaryJones commented Dec 30, 2016 •

edited

Loading

jrfnl commented Dec 30, 2016 •

edited

Loading