-
Notifications
You must be signed in to change notification settings - Fork 1
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
Consider ignoring lines with special cspell ignore comments #16
Comments
PHP_CodeSniffer would complain also about commented out code that does not end with a period. That happens because PHP_CodeSniffer expects comments to be a sentence commenting existing code, but that is not always true. |
@klonos this is our class for inline comments: https://github.com/backdrop-ops/phpcs/blob/main/Backdrop/Sniffs/Commenting/InlineCommentSniff.php So we can get phpcs to ignore cspell lines, just need a PR for that. |
@indigoxela I'm afraid that this is beyond me. I did have a look through the code though, and noticed these lines: // Only check the end of comment character if the start of the comment
// is a letter, indicating that the comment is just standard text.
// Also, when the comment starts with cspell: don't check the end of the
// comment.
if (preg_match('/^\p{L}/u', $commentText) === 1
&& strpos($commentText, 'cspell:') !== 0
) { So something must not be working as expected there - I just cannot figure out what that is 🤔 |
...I also noticed this: // Finally, the line below the last comment cannot be empty if this inline
// comment is on a line by itself.
if ($tokens[$previousContent]['line'] < $tokens[$stackPtr]['line']) { This should also account for // cspell:disable
$f = _filter_htmlcorrector('<p>دروبال');
$this->assertEqual($f, '<p>دروبال</p>', 'HTML corrector -- Encoding is correctly kept.');
// cspell:enable
$f = _filter_htmlcorrector('<script type="text/javascript">alert("test")</script>');
$this->assertEqual($f, '<script type="text/javascript"> |
I'm not really sure, what you're after, as cspell comments are ignored. What you're seeing is that an inline comment must not be followed by a blank line, which is something completely different. Removing the blank line fixes that. There isn't any nagging with cspell comment. Has never been, as this exclusion of cspell was in the initial commit of the sniff. So I don't actually get the issue description TBH. Mind to overhaul it, so it reflects what you're trying to achieve? |
There is also what backdrop/backdrop#4580 fixed. if (preg_match('/[\x{80}-\x{A0}' . // Non-printable ISO-8859-1 + NBSP.
'\x{AD}' . // Soft-hyphen.
'\x{2000}-\x{200F}' . // Various space characters.
// Omissis
'\x{FF01}-\x{FF60}' . // Full-width latin.
'\x{FFF9}-\x{FFFD}' . // Replacement characters.
'\x{0}-\x{1F}]/u', // NULL byte and control characters.
// cspell:enable
$name)) { Usually, CSpell comments are preceded by an empty line and may be followed by an empty line. |
@avpaderno your example has nothing to do with cspell comments, though. Maybe open a different issue for that? If this issue should be about allowing empty lines under circumstances - that's not clear from the issue description. |
@indigoxela The line causing the error is |
In fact, it doesn't. It's the newline after the inline comment, which isn't actually necessary for cspell. If you want to fix that (allow newline), just go for it. But the issue description needs an update. That's all I'm asking for. The issue description should ... describe the issue and what needs fixing. 😉 |
Anyway, I am not saying there is something that must be fixed. I think it is a specific case that could be ignored. |
It is not CSpell that shows that error; it is PHP_CodeSniffer. The error code does not say anything about the new line, but the last character in a comment (Inline comments must end in …). |
Here's a quick example that completely passes in phpcs - in our ruleset. <?php
/**
* @file
*/
/**
* Foo.
*/
function foo() {
// cspell:disable
$f = _filter_htmlcorrector('<p>دروبال');
$this->assertEqual($f, '<p>دروبال</p>', 'HTML corrector -- Encoding is correctly kept.');
// cspell:enable
$f = _filter_htmlcorrector('<script type="text/javascript">alert("test")</script>');
$this->assertEqual($f, '<script type="text/javascript">');
} ^^ that does not cause any phpcs nagging. Never did. That sniff has been forked from the Drupal ruleset with some modifications. |
Right, and it passes PHP_CodeSniffer rules because there is only a comment. // cspell:disable
$f = _filter_htmlcorrector('<p>دروبال'); Add more comments before that one and PHP_CodeSniffer will complain. // Example comment ending with a period.
// Another example comment that in real code should be a sentence.
// cspell:disable
$f = _filter_htmlcorrector('<p>دروبال'); |
The same error is reported with Drupal code when the Drupal and DrupalPractice rulesets are used. |
Again, to fix any problem look at the linked line of code. PR welcome. But please, can either of you update the issue description to actually describe the problem? @klonos @avpaderno |
Here's a PR that enables additional inline comments before cspell. Note that this is just one of the problems described here and has nothing to do with anything described in the initial report. Nor does it fix anything re newline-after-comment nagging. Something like this now passes:
|
Over in backdrop/backdrop-issues#6255, we have merged a few PRs that have added the following ignore lines in various files in our codebase:
As I have posted in backdrop/backdrop-issues#6302 (comment), PHPCS never complained about any of these additions, despite failing our regular coding standards around inline comments. Specifically, these lines:
Having said that:
So questions:
Should we add an exception in our PHPCS configuration to allow these specific lines to be as they are? ...or should we add periods to them across our codebase?
Although I have tested and confirmed that adding a period to these lines stops PHPCS from complaining, I didn't change the letter c in
// cspell:
to uppercase. Should we or not? For reference: https://cspell.org/configuration/document-settings shows the following formats for these lines:cSpell:disable
(second letter uppercase - no space after the:
)spell-checker: disable
(when the full word is used, a space follows:
to separate it from thedisable
/enable
/disable-next-line
keywords)spellchecker: disable
(same as previous format, but without the dash)So I am not sure if changing the format to
// Cspell:*
to make PHPCS happy will break CSpell or not 🤷🏼The text was updated successfully, but these errors were encountered: