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

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

Merged
merged 2 commits into from Jan 7, 2017

Conversation

Projects
None yet
7 participants
@jrfnl
Contributor

jrfnl commented Dec 25, 2016

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

$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 ) ) {

This comment has been minimized.

@GaryJones

GaryJones Dec 26, 2016

Member

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 ;-)
*/
@GaryJones

GaryJones Dec 26, 2016

Member

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 ;-)
*/

This comment has been minimized.

@jrfnl

jrfnl Dec 26, 2016

Contributor

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.

@jrfnl

jrfnl Dec 26, 2016

Contributor

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.

Show outdated Hide outdated 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 ),

This comment has been minimized.

@GaryJones

GaryJones Dec 26, 2016

Member

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

@GaryJones

GaryJones Dec 26, 2016

Member

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

This comment has been minimized.

@jrfnl

jrfnl Dec 26, 2016

Contributor

Fixed

@jrfnl

jrfnl Dec 26, 2016

Contributor

Fixed

@GaryJones

This comment has been minimized.

Show comment
Hide comment
@GaryJones

GaryJones Dec 26, 2016

Member

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.

Member

GaryJones commented Dec 26, 2016

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.

Add checking for translators comments to the I18n sniff.
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

This comment has been minimized.

Show comment
Hide comment
@jrfnl

jrfnl Dec 29, 2016

Contributor

Rebased after #733 to allow unit tests to pass.

Contributor

jrfnl commented Dec 29, 2016

Rebased after #733 to allow unit tests to pass.

@ramiy

This comment has been minimized.

Show comment
Hide comment
@ramiy

ramiy Dec 30, 2016

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.

ramiy commented Dec 30, 2016

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

This comment has been minimized.

Show comment
Hide comment
@jrfnl

jrfnl Dec 30, 2016

Contributor

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.
Contributor

jrfnl commented Dec 30, 2016

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

This comment has been minimized.

Show comment
Hide comment
@jrfnl

jrfnl Dec 30, 2016

Contributor

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.

Contributor

jrfnl commented Dec 30, 2016

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

This comment has been minimized.

Show comment
Hide comment
@grappler

grappler Dec 30, 2016

Contributor

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.

Contributor

grappler commented Dec 30, 2016

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

This comment has been minimized.

Show comment
Hide comment
@ntwb

ntwb Dec 30, 2016

Member

... 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

Member

ntwb commented Dec 30, 2016

... 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

This comment has been minimized.

Show comment
Hide comment
@ntwb

ntwb Dec 30, 2016

Member

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

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

Member

ntwb commented Dec 30, 2016

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

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

@GaryJones

This comment has been minimized.

Show comment
Hide comment
@GaryJones

GaryJones Dec 30, 2016

Member

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.

Member

GaryJones commented Dec 30, 2016

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

This comment has been minimized.

Show comment
Hide comment
@ocean90

ocean90 Dec 30, 2016

Contributor

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/.

Contributor

ocean90 commented Dec 30, 2016

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

This comment has been minimized.

Show comment
Hide comment
@jrfnl

jrfnl Dec 30, 2016

Contributor

@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.

Contributor

jrfnl commented Dec 30, 2016

@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.

@jrfnl

This comment has been minimized.

Show comment
Hide comment
@jrfnl

jrfnl Dec 30, 2016

Contributor

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 ?

Contributor

jrfnl commented Dec 30, 2016

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

This comment has been minimized.

Show comment
Hide comment
@vslavik

vslavik Dec 31, 2016

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.

vslavik commented Dec 31, 2016

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

This comment has been minimized.

Show comment
Hide comment
@jrfnl

jrfnl Dec 31, 2016

Contributor

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

Contributor

jrfnl commented Dec 31, 2016

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

@jrfnl

This comment has been minimized.

Show comment
Hide comment
@jrfnl

jrfnl Jan 5, 2017

Contributor

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

Contributor

jrfnl commented Jan 5, 2017

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

@jrfnl jrfnl merged commit 7f4c535 into develop Jan 7, 2017

2 checks passed

continuous-integration/travis-ci/pr The Travis CI build passed
Details
continuous-integration/travis-ci/push The Travis CI build passed
Details

@jrfnl jrfnl deleted the feature/issue-423-check-translator-comment branch Jan 7, 2017

@jrfnl jrfnl referenced this pull request Mar 6, 2017

Merged

Changelog for version 0.11.0 #866

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