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

Allow translators: comments. #562

Merged

Conversation

jrfnl
Copy link
Member

@jrfnl jrfnl commented May 2, 2016

A bugfix upstream now causes Doc comment short description must start with a capital letter errors for /* translators: ... */ comments.

Excluding the offending rule to continue to allow these type of comments.

Ref: squizlabs/PHP_CodeSniffer#855

@westonruter
Copy link
Member

@jrfnl I'm surprised that PHPCS is reporting an error for this since such translators comments aren't in phpdoc block comments (/** ... */) but just vanilla multiline comments (/* ... */). Shouldn't PHPCS be skipping phpdoc validation for the latter?

@jrfnl
Copy link
Member Author

jrfnl commented May 2, 2016

That seems to me a discussion to take upstream.
However, in the mean time, WPCS checked builds should pass when they contain translators comments...

If and when upstream would decide to change this, we can always reverse the exclusion.

@westonruter
Copy link
Member

@jrfnl I cannot reproduce the issue, however.

If I do:

phpcs --standard=WordPress-Docs --sniffs=Generic.Commenting.DocComment

It only complains about the second instance, not the first:

/* translators: 1: the id argument, 2: sidebar name, 3: recommended id value */
_doing_it_wrong( __FUNCTION__, sprintf( __( 'No %1$s was set in the arguments array for the "%2$s" sidebar. Defaulting to "%3$s". Manually set the %1$s to "%3$s" to silence this notice and keep existing sidebar content.' ), '<code>id</code>', $sidebar['name'], $sidebar['id'] ), '4.2.0' );

/** translators: 1: the id argument, 2: sidebar name, 3: recommended id value */
_doing_it_wrong( __FUNCTION__, sprintf( __( 'No %1$s was set in the arguments array for the "%2$s" sidebar. Defaulting to "%3$s". Manually set the %1$s to "%3$s" to silence this notice and keep existing sidebar content.' ), '<code>id</code>', $sidebar['name'], $sidebar['id'] ), '4.2.0' );

@jrfnl
Copy link
Member Author

jrfnl commented May 2, 2016

@westonruter You can see the issue with /* */ syntax here:
https://travis-ci.org/tollmanz/debug-bar-cron/jobs/127228904 which refers to this line of code:

/* translators: %s is number of seconds. */

But yes, there is something funky going on with the sniff as it seems to only pick up the second instance.
In the file the above code comes from, it's not complaining about the same syntax earlier on in the file on line 385

@westonruter
Copy link
Member

I can't reproduce the issue on that line either. Try this:

$ curl -sG https://raw.githubusercontent.com/tollmanz/debug-bar-cron/2f4173b043c1b04b64ad199d768c4294b9f75288/class-debug-bar-cron.php | phpcs --standard=WordPress-Docs -v --sniffs=Generic.Commenting.DocComment
Registering sniffs in the WordPress Docs standard... DONE (1 sniffs registered)
Creating file list... DONE (0 files in queue)
Processing STDIN [PHP => 4436 tokens in 638 lines]... DONE in 82ms (0 errors, 0 warnings)
Time: 961ms; Memory: 9Mb

@westonruter
Copy link
Member

Actually, the error being reported is a different issue than Generic.Commenting.DocComment. It seems to actually be the Squiz.Commenting.InlineComment sniff:

$ curl -sG https://raw.githubusercontent.com/tollmanz/debug-bar-cron/2f4173b043c1b04b64ad199d768c4294b9f75288/class-debug-bar-cron.php | phpcs --standard=WordPress-Docs -s

FILE: STDIN
----------------------------------------------------------------------
FOUND 1 ERROR AFFECTING 1 LINE
----------------------------------------------------------------------
 454 | ERROR | Inline comments must start with a capital letter
     |       | (Squiz.Commenting.InlineComment.NotCapital)
----------------------------------------------------------------------

A bugfix upstream now causes `Doc comment short description must start with a capital letter` errors for `/* translators: ... */` comments.

Excluding the offending rule to continue to allow these type of comments.
@jrfnl
Copy link
Member Author

jrfnl commented May 2, 2016

Even weirder as I'm pretty sure I copied & pasted it from my locally run log which would indicate two different sniffs being potentially buggy for the translators comments.... or did I ? Where's my head ? Anyone seen it ? If you find it, give me a shout will ya ?

In the mean time: adjusted the PR to address the right sniff.

@jrfnl jrfnl force-pushed the feature/fix-overzealous-doc-sniff branch from 5223ed4 to 0eabc71 Compare May 2, 2016 21:13
@westonruter westonruter merged commit caf19ca into WordPress:develop May 2, 2016
@jrfnl jrfnl deleted the feature/fix-overzealous-doc-sniff branch May 2, 2016 22:33
@jrfnl jrfnl added this to the 0.10.0 milestone Aug 27, 2016
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.

2 participants