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

[Docs] Add XML doc for Squiz.Commenting.BlockComment sniff #848

Open
wants to merge 14 commits into
base: master
Choose a base branch
from

Conversation

costdev
Copy link

@costdev costdev commented Feb 27, 2025

Description

Adds XML doc for the Squiz\Commenting\BlockComment sniff.

Suggested changelog entry

Add XML doc for Squiz.Commenting.BlockComment sniff

Related issues/external references

Partial fix for #148

Types of changes

  • Bug fix (non-breaking change which fixes an issue)
  • New feature (non-breaking change which adds functionality)
  • Breaking change (fix or feature that would cause existing functionality to change)
    • This change is only breaking for integrators, not for external standards or end-users.
  • Documentation improvement

PR checklist

  • I have checked there is no other PR open for the same change.
  • I have read the Contribution Guidelines.
  • I grant the project the right to include and distribute the code under the BSD-3-Clause license (and I have the right to grant these rights).
  • I have added tests to cover my changes.
  • I have verified that the code complies with the projects coding standards.
  • [Required for new sniffs] I have added XML documentation for the sniff.

Copy link
Member

@jrfnl jrfnl left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Hi @costdev Welcome and thank you for your willingness to contribute to PHP_CodeSniffer!

I love the effort you've put into this one. Having said that, the doc is quite large now, which makes me wonder if it would be possible to convey the same information in a slightly more succinct manner to improve the chances of it actually getting read 😉

Here are some suggestions to consider, but please think it over yourself and feel free to deviate:

  • What about including a short description of what is considered a "block comment" in the first <standard> block ?
  • First <standard> has five code comparisons - could the same information be conveyed in maybe two code comparisons ?
    And maybe one of the remaining "valid" comparisons could use aligned stars at the start of each line for variation ?
  • What about combining the "Block comment text should start on a new line." and the "The first line should not be empty." standards as they kind of enforce the same thing ?
  • I seem to be missing some guidance on the indentation requirements for the star of the docblock closer (LastLineIndent error code) ?

I also wonder what guided the order of the <standard> blocks. To me, it feels a bit all over the place, jumping from start of block comment formatting to end formatting, to within formatting, to before formatting etc...
I can imagine the documentation may be easier to take in if the rules were ordered to follow the structure of the docblock ?

Lastly, the <em> tags should be used in both the "valid" as well as the "invalid" code samples. They seem to be missing from the "valid" ones now.

Let me know if you have any questions, if not, I look forward to the next iteration.

</code_comparison>
<standard>
<![CDATA[
The block comment closer should be on a new line.
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I haven't checked, but should it just be on a new line or should it be on a line by itself ? (i.e. with nothing after it either)

Copy link
Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

For example:

<?php
/*
 * A block comment
 * with multiple lines.
 *
 * And a second paragraph.
 */doSomething();

This triggers Squiz.Commenting.BlockComment.NoEmptyLineAfter.

Do you think an example of this should be included in a code comparison here, or should I add a second example (in the same code comparison block) here?

]]>
</code>
</code_comparison>
</documentation>
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Please end the file on a new line.

@costdev
Copy link
Author

costdev commented Mar 8, 2025

What about including a short description of what is considered a "block comment" in the first <standard> block ?

Sure!

First <standard> has five code comparisons - could the same information be conveyed in maybe two code comparisons ?
And maybe one of the remaining "valid" comparisons could use aligned stars at the start of each line for variation ?

That sounds good. I can merge the /** */, /** **/, /* **/ into the first "invalid" comparison and //, # into the second, and use aligned stars in the second "valid" comparison.

What about combining the "Block comment text should start on a new line." and the "The first line should not be empty." standards as they kind of enforce the same thing ?

While they enforce separate things, for documentation it's probably not necessary to be as verbose as I was in separating these two. I think "Block comment text should start on a new line." probably covers it well, so I can even just remove "The first line should not be empty.".

I seem to be missing some guidance on the indentation requirements for the star of the docblock closer (LastLineIndent error code) ?

I wasn't totally sure about covering this one separately, as it felt covered by the "If there are no asterisks" and "If asterisks are used" entries. Would you like me to cover this one separately anyway?

I also wonder what guided the order of the <standard> blocks. To me, it feels a bit all over the place, jumping from start of block comment formatting to end formatting, to within formatting, to before formatting etc... I can imagine the documentation may be easier to take in if the rules were ordered to follow the structure of the docblock ?

Originally, I was following the order of the errors in the sniff. Afterwards, I felt they were a bit all over the place, so I tried another order which focused on establishing correct openers and closers, then addressed the contents of the comment. Since that too feels a bit all over the place, I'm happy to try a different order.

How does this sound?

A block comment immediately after an open tag should not have a preceeding blank line.
A block comment should use /* */.
A block comment should not be empty.
Single line block comments are not allowed.
Block comment text should start on a new line.
A block comment should start with a capital letter.
If there are no asterisks at the start of each line, the contents of the docblock should be indented by at least 4 spaces.
If asterisks are used, they should be aligned.
The block comment closer should be on a new line.
There should be an empty line after the block comment.

Lastly, the <em> tags should be used in both the "valid" as well as the "invalid" code samples. They seem to be missing from the "valid" ones now.

Woops. I got it into my head that only the "invalid" needed to be highlighted. No idea how that happened since it's explicitly stated in the contributing guidelines. 😅

@costdev
Copy link
Author

costdev commented Mar 8, 2025

Follow-up on the explanation of a block comment:

I want to make sure I capture the right concept in the explanation.

  • They're definitely used to provide a longer explanation of code.
  • They're also sometimes used to draw more attention to a piece of code than a single-line comment would, even if the block comment only has one line of text inside it. I'm not sure if this is strictly relevant here though.
  • While /* */ are commonly used to comment out blocks of code, that's not really relevant here. It's commented out code, not a block comment.

If you agree with the above, what do you think of this for the first <standard> block?

A block comment contains multiple lines and provides a longer explanation of code, or draws more attention to it than a single-line comment would.
    
Block comments should use /* */. <-- This would of course be replaced by whatever becomes our new first <standard> block.

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

Successfully merging this pull request may close these issues.

2 participants