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

Add a translator comment sniff #423

Closed
atimmer opened this issue Aug 18, 2015 · 12 comments · Fixed by #742
Closed

Add a translator comment sniff #423

atimmer opened this issue Aug 18, 2015 · 12 comments · Fixed by #742

Comments

@atimmer
Copy link
Member

atimmer commented Aug 18, 2015

All strings with placeholders should have a translator comment to explain what the placeholders are replaced with. It would be very nice to have a sniff to automatically flag strings without a proper translator comment.

These would be flagged as incorrect:

__( 'A %1$s to be translated', 'text domain' );

/* translators: %2$s is replaced with the number of translations */
__( 'A %1$s to be translated', 'text domain' );

And these would be fine:

__( 'A string to be translated', 'text domain' );

/* translators: %1$s is replaced with "string" */
__( 'A %1$s to be translated', 'text domain' );

Strings can be checked for placeholders using the following regex: %(\d+\$(?:\d+)?)?[bcdefgosuxEFGX].

@westonruter
Copy link
Member

👍

@johnbillion
Copy link
Member

I'm not 100% sure, but I believe subsequent instances of an identical string should not be proceeded by the translator comment (in the same way subsequent uses of a hook or filter shouldn't be documented). Needs checking.

@atimmer
Copy link
Member Author

atimmer commented Oct 7, 2015

@johnbillion This depends on whether or not the pot generation will correctly pickup the translator comment if it is only on one of the strings.

If that's the case there is still value in this sniff, there then needs to be a way to tell it that the translation has been documented elsewhere.

@JDGrimes
Copy link
Contributor

JDGrimes commented Aug 1, 2016

Related: #567

@jrfnl
Copy link
Member

jrfnl commented Dec 24, 2016

All, I've started working on adding this check, but would like to verify the exact specs.

This is what I've found so far:


It has to start with the words translators: and to be the last PHP comment before the gettext call.

Source: https://codex.wordpress.org/I18n_for_WordPress_Developers#Descriptions

So that translators know how to translate a string like __( 'g:i:s a' ) you can add a clarifying comment in the source code. It has to start with the words translators: and to be the last PHP comment before the gettext call. Here is an example:

/* translators: draft saved date format, see http://php.net/date */
$saved_date_format = __( 'g:i:s a' );

It’s also used to explain placeholders in a string like _n_noop( 'Version %1$s addressed %2$s bug.','Version %1$s addressed %2$s bugs.' ).

/* translators: 1: WordPress version number, 2: plural number of bugs. */
_n_noop( '<strong>Version %1$s</strong> addressed %2$s bug.',
        '<strong>Version %1$s</strong> addressed %2$s bugs.' );

Source: https://developer.wordpress.org/plugins/internationalization/how-to-internationalize-your-plugin/#descriptions (similar to: https://developer.wordpress.org/themes/functionality/internationalization/#descriptions)


However for a sniff those specs are a bit limited. So I have the following questions:

  • What type of comments are acceptable ? Only single line star comments /* */ as per the examples ? Or also // single line comments, multi-line star comments, doc blocks ?
// translators:

/* transators: */

/* translators: a reeeeeaaaaaaaaaallllllllllllllllllllllllllllllllllllly looooonnnnnnnnnnnnnggggggggggg
   comment for translators. */

/*
 * translators:
 */

/**
 * translators:
*/
  • "must be the last PHP comment before the gettext call" This is open to interpretation - the last comment could be five lines above the translation call. In practice I've found that PoEdit will only pick up the translators comment if it's on the line right above the translators call. I'm not sure about other makepot tools. I'm inclined to make this quite a strict check where it is required for the comment to be on the line above the gettext call.
printf(
	/* translators: number of monkeys, location. */
	__( 'There are %1$d monkeys in the %2$s', 'my-slug' ),
	intval( $number ),
	esc_html( $string )
); // This would be ok.

/* translators: number of monkeys, location. */
printf(
	__( 'There are %1$d monkeys in the %2$s', 'my-slug' ),
	intval( $number ),
	esc_html( $string )
); // This would NOT be ok.

Other than that I am making the following presumptions:

  • Translators comments are required when any of the text strings (text, single, plural) contains a placeholder.
  • If a translators comment is present, but there is no placeholder detected, do nothing. (i.e. the time format example above)

I will currently not address checking the number of placeholders against the translators comment as the formats used vary and this would IMHO be extremely prone to bugs. See the below examples to illustrate what I mean:

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

@jrfnl
Copy link
Member

jrfnl commented Dec 24, 2016

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 !

@jrfnl
Copy link
Member

jrfnl commented Dec 24, 2016

And another question: error or warning ?

@GaryJones
Copy link
Member

  1. The default in core seems to be /* translators: ... */, though I know we have to disable a comment check for using a multiline comment format on a single line. // translators: will work, and should be allowed too.

  2. Agreed that it only seems to be the line immediately above (though I any blank lines between the comment and string may be ignored), so that's what we should allow. Poor wording in the spec.

  3. There will be a lot of old code without translator comments, so I'd say Warning, and make it be recommended but not required (or required, under WordPress-Extra), if placeholders exist.

@jrfnl
Copy link
Member

jrfnl commented Dec 24, 2016

@GaryJones Good input. Thanks!

The questions regarding the documentation style and whether or not new lines are allowed is not just one of personal preference/"what does core do". I think we also need to look at how tools to generate .pot files handle this.

I can think of a few:

  • PoEdit
  • makepot

But there are bound to be more tools out there and what about GlotPress ?

Anyone here who can give us some insight into the inner workings of these tools with regards to comment style and comment position ? or know of someone who does know ? If so, please ping them to alert them that their input would be much appreciated.

@GaryJones
Copy link
Member

@ocean90 will know if GlotPress has any considerations here, or whether it internally uses makepot or similar.

@grappler
Copy link
Member

grappler commented Dec 24, 2016

Glotpress uses Makepot.

If I am not mistaken then the code that fetched the comments is in find_function_calls() in https://i18n.svn.wordpress.org/tools/trunk/extract/extract.php which uses T_COMMENT

This core ticket maybe of interest too. https://core.trac.wordpress.org/ticket/39116

@jrfnl
Copy link
Member

jrfnl commented Dec 25, 2016

PR #742 is ready for testing ;-)

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

Successfully merging a pull request may close this issue.

7 participants