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 Squiz.Commenting.DocCommentAlignment sniff #8

Open
ghost opened this issue Mar 1, 2023 · 11 comments · May be fixed by #9
Open

Add Squiz.Commenting.DocCommentAlignment sniff #8

ghost opened this issue Mar 1, 2023 · 11 comments · May be fixed by #9
Labels
enhancement New feature or request

Comments

@ghost
Copy link

ghost commented Mar 1, 2023

I saw this issue in a file that wasn't picked up by any of the current sniffs:

  /**
 * Implements BackdropCacheInterface::deleteMultiple().
 */
  public function deleteMultiple(array $cids) {

This will be flagged if we add the 'Squiz.Commenting.DocCommentAlignment' sniff.

@ghost ghost added the enhancement New feature or request label Mar 1, 2023
ghost pushed a commit that referenced this issue Mar 1, 2023
@ghost ghost linked a pull request Mar 1, 2023 that will close this issue
@ghost
Copy link
Author

ghost commented Mar 1, 2023

Hmm, but then that sniff flags the following code:

   * @param string $key
   *   A string that maps to a key within the configuration data.
   *   For instance in the following configuration array:
   *   @code
   *   array(
   *     'foo' => array(
   *       'bar' => 'baz',
   *     ),
   *   );
   *   @endcode
   *   A key of 'foo.bar' would return the string 'baz'. However, a key of 'foo'
   *   would return array('bar' => 'baz').
   *   If no key is specified, then the entire data array is returned.

It says the @code tags should be out-dented:

   * @param string $key
   *   A string that maps to a key within the configuration data.
   *   For instance in the following configuration array:
   * @code
   *   array(
   *     'foo' => array(
   *       'bar' => 'baz',
   *     ),
   *   );
   * @endcode

So might need to make our own sniff for this...

@ghost
Copy link
Author

ghost commented Mar 1, 2023

Ah ha! Worked out how to exclude that one sniff message that was causing issues:

<rule ref="Squiz.Commenting.DocCommentAlignment">
  <!-- Doesn't allow nested tags. -->
  <exclude name="Squiz.Commenting.DocCommentAlignment.SpaceAfterStar"/>
</rule>

I also re-arranged the ruleset.xml slightly so that all Squiz sniffs are listed together, rather than having regular ones up top and then overridden ones below (makes it easier to find what you're looking for if they're all together).

@indigoxela
Copy link
Collaborator

I'll try to test ASAP. Yes, some of the Squiz rules overlap with the one(s) forked from Drupal, so extra care has to be taken. At least, we don't want to produce false positives. 😉

@indigoxela
Copy link
Collaborator

indigoxela commented Mar 1, 2023

@BWPanda are you sure this behaves as intended? If I'm doing it wrong like:

 /**
 *
 */
function foobar(array $arr) {
  return TRUE;
}

I get:

FOUND 2 ERRORS AFFECTING 2 LINES
----------------------------------------------------------------------
 8 | ERROR | [x] Expected 2 space(s) before asterisk; 1 found
 9 | ERROR | [x] Expected 2 space(s) before asterisk; 1 found
----------------------------------------------------------------------

That seems a bit misleading, as the problem is in line 7 (indented one space, but shouldn't be).

@ghost
Copy link
Author

ghost commented Mar 1, 2023

This addresses issues where the docblock's opening tag is aligned properly but the rest of the docblock isn't.

For issues like your example where the opening tag of a docblock is misaligned to the function, we'll need to find a sniff for that too I guess.

But that could be a separate issue and doesn't mean this PR doesn't work.

@indigoxela
Copy link
Collaborator

we'll need to find a sniff for that too I guess. ... But that could be a separate issue

I partly agree, we should figure out how to address this docblock misalignment. But ... as your PR only partly addresses it, this leads to a WTF with misleading messages. Maybe we can fix that in a way, that doesn't introduce confusion?

Here's the source code of that sniff: https://github.com/squizlabs/PHP_CodeSniffer/blob/master/src/Standards/Squiz/Sniffs/Commenting/DocCommentAlignmentSniff.php

Maybe we find something in Drupal-land: https://github.com/pfrenssen/coder

@ghost
Copy link
Author

ghost commented Mar 4, 2023

@indigoxela Thanks for the tip. I found that running Drupal's sniffs on a test file flagged the error we're having. The related sniff was one of their custom ones (that they've forked from squizlabs). So I copied it to our sniffs and it seems to work (PR updated):

<?php
/**
 * @file
 * A test file for PHPCS.
 */

 /**
  * This function returns 'blah'.
  */
function tester() {
  return 'blah';
}

/**
* This function returns 'yada'.
*/
function tester2() {
  return 'yada';
}

Returns:

----------------------------------------------------------------------
FOUND 3 ERRORS AFFECTING 3 LINES
----------------------------------------------------------------------
  7 | ERROR | [x] Line indented incorrectly; expected 0 spaces, found
    |       |     1
    |       |     (Backdrop.WhiteSpace.ScopeIndent.IncorrectExact)
 15 | ERROR | [x] Expected 1 space(s) before asterisk; 0 found
    |       |     (Squiz.Commenting.DocCommentAlignment.SpaceBeforeStar)
 16 | ERROR | [x] Expected 1 space(s) before asterisk; 0 found
    |       |     (Squiz.Commenting.DocCommentAlignment.SpaceBeforeStar)
----------------------------------------------------------------------

It doesn't flag the other lines in the same docblock, unless you fix just the first one then run it again. But I think that's fine (most people would notice the issue and just fix the entire docblock).

@indigoxela
Copy link
Collaborator

indigoxela commented Mar 4, 2023

At first I was exited, but then... this flags more than just comments.

An example:

function foo() {
  db_update('i18n_string')
  ->fields($update + array(
    'type' => $source->type,
    'objectid' => $source->objectid,
    'property' => $source->property,
    'objectindex' => $source->objectindex,
  ))
  ->condition('lid', $source->lid)
  ->execute();
}

Do we have definitions for that in Backdrop coding standards? It might be that it actually makes sense that this is an error, but ...

Re the misleading nagging with misaligned function comments: if I get it right, we do not need Squiz.Commenting.DocCommentAlignment.SpaceAfterStar anymore? Or do we?

@ghost
Copy link
Author

ghost commented Mar 16, 2023

@indigoxela So I was a bit confused by all this, and decided to try building a standard from scratch to see what sniffs we needed and which we didn't. Here's what I've spent some time over the last few weeks doing:

  • Setup a new, blank standard that includes all Generic and Squiz sniffs
  • Run that over Backdrop core
  • For each issue flagged that was a legitimate issue with our code, I noted that sniff/message as a good one
  • For each issue flagged that isn't a problem with our code, I excluded that sniff/message (i.e. it's a bad one)

This eventually helped me to come up with a list of good and bad sniffs.

Of the ~119 sniff messages in the Generic standard, I excluded ~55 bad ones.
Of the ~323 sniff messages in the Squiz standard (that weren't already included in the Generic standard), I excluded ~92 bad ones.

So as you can see, it seems to be better to include the full Generic and Squiz standards, then exclude specific sniffs/messages that don't match our coding standards (rather than just including specific sniffs as we do now).

Would you like my new Standard as a PR here, or in a new issue? I think it addresses the issues here we've been discussing here:

<?php
/**
 * @file
 * A test file for PHPCS.
 */

 /**
  * This function returns 'blah'.
  */
function tester() {
  return 'blah';
}

/**
* This function returns 'yada'.
*/
function tester2() {
  return 'yada';
}

function foo() {
  db_update('i18n_string')
    ->fields($update + array(
      'type' => $source->type,
      'objectid' => $source->objectid,
      'property' => $source->property,
      'objectindex' => $source->objectindex,
    ))
    ->condition('lid', $source->lid)
    ->execute();
}
FILE: /app/test/test.php
-----------------------------------------------------------------------------------------------
FOUND 6 ERRORS AFFECTING 4 LINES
-----------------------------------------------------------------------------------------------
  9 | ERROR | [ ] Missing @return tag in function comment (Squiz.Commenting.FunctionComment.MissingReturn)
 15 | ERROR | [x] Expected 1 space(s) before asterisk; 0 found
    |       |     (Squiz.Commenting.DocCommentAlignment.SpaceBeforeStar)
 16 | ERROR | [x] Expected 1 space(s) before asterisk; 0 found
    |       |     (Squiz.Commenting.DocCommentAlignment.SpaceBeforeStar)
 16 | ERROR | [ ] Missing @return tag in function comment (Squiz.Commenting.FunctionComment.MissingReturn)
 21 | ERROR | [x] Missing function doc comment (Backdrop.Commenting.FunctionComment.Missing)
 21 | ERROR | [ ] Missing doc comment for function foo() (Squiz.Commenting.FunctionComment.Missing)
-----------------------------------------------------------------------------------------------

@indigoxela
Copy link
Collaborator

Wow, thank you so much, to put all that effort here. 🙏

If you prefer to attach your PR here, it might make sense to adapt the issue description. Or start a new one - whatever seems better.

@ghost
Copy link
Author

ghost commented Mar 16, 2023

I decided a new issue might be better, since this is a fairly significant change. We can always close this issue later if it's no longer necessary if/when the other PR is merged.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
enhancement New feature or request
Projects
None yet
Development

Successfully merging a pull request may close this issue.

1 participant