Consider ignoring lines with special cspell ignore comments

[klonos]

Over in https://github.com/backdrop/backdrop-issues/issues/6255, we have merged a few PRs that have added the following ignore lines in various files in our codebase:

// cspell:disable-next-line
// cspell:disable
// cspell:enable

As I have posted in https://github.com/backdrop/backdrop-issues/issues/6302#issuecomment-1826415702, PHPCS never complained about any of these additions, despite failing our regular coding standards around inline comments. Specifically, these lines:

• start with a lowercase letter
• do not end with a period or any of the allowed comment ending characters

Having said that:

• these aren't regular comments - they have a special purpose, which is to ignore lines when doing a spell check on the PR changes.
• PHPCS with its current configuration only complains about the missing ending period - not about the first letter being lowercase.

So questions:

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

2. 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 the disable/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 🤷🏼

[avpaderno]

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. I would rather ignore that, or avoid that warning is given.

[indigoxela]

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

[klonos]

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

[klonos]

...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:enable comments, which may need to be added right at the end of a chunk of code that is followed by an empty line. Like this example:

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

[indigoxela]

So something must not be working as expected there ...

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?

[avpaderno]

There is also what https://github.com/backdrop/backdrop/pull/4580 fixed.
In the screenshot shown in that PR, GitHub Actions reports that Inline comments must end in full-stops, exclamation marks, question marks, colons, or closing parentheses, but I think the reason for the report is that there are two consecutive comments.

  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.

[indigoxela]

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

[avpaderno]

@indigoxela The line causing the error is // cspell:enable which contains a CSpell comment.

[indigoxela]

The line causing the error is // cspell:enable which contains a CSpell comment.

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

[avpaderno]

Anyway, I am not saying there is something that must be fixed. I think it is a specific case that could be ignored.
I have seen that Inline comments must end in full-stops, exclamation marks, question marks, colons, or closing parentheses error shown by PHP_CodeSniffer for commented out code. Even in that case, the error was caused by consecutive commented out lines. (A single commented out line would not cause that error.)

[avpaderno]

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

[indigoxela]

It is not CSpell that shows that error; it is PHP_CodeSniffer.

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. Here again a link to our ruleset, where cspell comments are ignored: https://github.com/backdrop-ops/phpcs/blob/main/Backdrop/Sniffs/Commenting/InlineCommentSniff.php#L366 That line was always there since this rule has been added.

That sniff has been forked from the Drupal ruleset with some modifications.

[avpaderno]

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>دروبال');

[avpaderno]

The same error is reported with Drupal code when the Drupal and DrupalPractice rulesets are used.
Drupal core uses its own ruleset which avoids that error, but I would not adopt the same ruleset just to avoid that error in this specific case.

[indigoxela]

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

[indigoxela]

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:

    // Some extra comment before that.
    // cspell:disable
    $f = _filter_htmlcorrector('<p>دروبال');