Generic/DocComment: add XML documentation #247

rodrigoprimo · 2024-01-10T19:59:45Z

Description

This PR adds the XML documentation for the Generic.Commenting.DocComment sniff.

This sniff contains several errors, so I opted to add a <standard> block at the top summarizing all the checks.

Several of the valid examples are the same, but what is being checked felt different enough to use different <standard> and <code_comparison> blocks. If we want to make this documentation shorter, we might group related errors in the same <standard> and <code_comparison> blocks. For example, the errors long and short descriptions must start with a capital letter, and there must be a blank line between different doc comment elements.

Suggested changelog entry

Add XML documentation for the Generic.Commenting.DocComment sniff.

Related issues/external references

Part of #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.

jrfnl

@rodrigoprimo Thank you for taking on this sniff ! Sorry it took me a while before reviewing it.

Pff... this is a long one. I think your suggestion to group a few of the checks makes sense. Would you be up for iterating on the current PR with a proposal for grouping the checks ?

One thing which I'd like to see updated throughout (with an exception for the <documentation title...> which should reflect the sniff name): the "formal" name for these type of comments is "DocBlock", not "Doc comment".

jrfnl · 2024-02-23T05:44:08Z

src/Standards/Generic/Docs/Commenting/DocCommentStandard.xml

+ /**
+ * Short description.
+<em></em>
+ */


This seems like a stray example. I realize that you are highlighting with and without the *, but IMO, that isn't really a significant enough difference to have a second code sample.

jrfnl · 2024-02-23T05:47:04Z

src/Standards/Generic/Docs/Commenting/DocCommentStandard.xml

+        ]]>
+        </code>
+        <code title="Invalid: Blank lines before the short description.">
+        <![CDATA[


Same remark about the second code sample.

jrfnl · 2024-02-23T05:50:16Z

src/Standards/Generic/Docs/Commenting/DocCommentStandard.xml

+    </code_comparison>
+    <standard>
+    <![CDATA[
+    There must be a single blank line after a tag group.


Suggested change

There must be a single blank line after a tag group.

Tag groups must be separated by a single blank line.

Reason for this subtle change: there should be no blank line at the end of a DocBlock (i.e. after the last tag group).

@jrfnl, I just realized that the error message is the same as what I had used in the documentation. Should I update it as well, either in this PR or in a separate PR?

The error code is called SpacingAfterTagGroup, but I guess that is harder to change as it would be a breaking change.

PHP_CodeSniffer/src/Standards/Generic/Sniffs/Commenting/DocCommentSniff.php

Lines 293 to 294 in 84acf4e

$error = 'There must be a single blank line after a tag group';

$fix = $phpcsFile->addFixableError($error, $lastTag, 'SpacingAfterTagGroup');

In my opinion, the error message in the sniff is fine as it is, as it won't be thrown when the tag group is the last content in the docblock, so won't cause confusion. That's different for the docs.

rodrigoprimo · 2024-02-28T19:44:27Z

Thanks for the review, @jrfnl! I have implemented the small changes that you requested. Next, I will look into grouping a few of the blocks. I will leave another comment once this PR is ready for another review.

rodrigoprimo · 2024-02-29T15:22:10Z

@jrfnl, I pushed a few new commits grouping some of the checks together. I created a separate commit for each of the new groups so that it is easier to discard any of the groupings that we might decide to discard.

Here is the list of groupings by error code:

LongNotCapital and ShortNotCapital
SpacingBeforeTags, SpacingAfterTagGroup and SpacingBetween
ContentAfterOpen and ContentBeforeClose
MissingShort and SpacingBeforeShort

This PR is ready for another review.

jrfnl

@rodrigoprimo Thank you for making those updates. I agree that the current groupings are good and the doc reads much easier now.

I've now gone through the PR with a fine toothcomb and left lots of nitty gritty comments.

Have a look at them and let me know what you think.

jrfnl · 2024-03-26T14:44:06Z

src/Standards/Generic/Docs/Commenting/DocCommentStandard.xml

+<documentation title="Doc Comment">
+    <standard>
+    <![CDATA[
+    Enforces several rules related to the formatting of DocBlocks in PHP code.


Suggested change

Enforces several rules related to the formatting of DocBlocks in PHP code.

Enforces rules related to the formatting of DocBlocks in PHP code.

The sniff is named "Doc Comment", while it really is about DocBlocks. Changing the name would be a BC-break, so is not on the table, but maybe it would be good to make it clear what a DocBlock is in this summary standard block and maybe also "explain" that the "Doc Comment" from the sniff name is really DocBlock.

This page might provide inspiration for creating a good explanation of what a DocBlock is: https://docs.phpdoc.org/3.0/guide/references/phpdoc/basic-syntax.html#what-is-a-docblock

I just pushed a new commit suggesting a new description for the sniff including the points that you mentioned. I'm afraid the result might be a bit too long. Please let me know what you think.

jrfnl · 2024-03-26T16:37:55Z

src/Standards/Generic/Docs/Commenting/DocCommentStandard.xml

+    ]]>
+    </standard>
+    <code_comparison>
+        <code title="Valid: The opening and closing DocBlock tags are the only content on the line.">


"on the line" is confusing as the line for the opening/closing tags will be on different lines.

Not sure if this is better, but maybe you want to have a think about the phrasing (here and in the "invalid" example too).

Suggested change

<code title="Valid: The opening and closing DocBlock tags are the only content on the line.">

<code title="Valid: The opening and closing DocBlock tags have to be on a line by themselves.">

Implemented in dc22715

src/Standards/Generic/Docs/Commenting/DocCommentStandard.xml

jrfnl · 2024-03-26T17:08:24Z

src/Standards/Generic/Docs/Commenting/DocCommentStandard.xml

+/**
+ * Short description.
+ *
+ * @deprecated <em>1.0.0</em>
+ * @return     <em>bool</em>
+ */


In a lot of coding standards, the @return tag is not allowed to be grouped with other tags, so, even though this sniff does not have any rules about that, we should maybe avoid doing that in the examples. (here and in other places)

What about this instead ?

Suggested change

/**

* Short description.

*

* @deprecated 1.0.0

* @return bool

*/

/**

* Short description.

*

* @since 0.5.3

* @deprecated 1.0.0

*/

Also implemented in dc22715

src/Standards/Generic/Docs/Commenting/DocCommentStandard.xml

jrfnl · 2024-03-26T17:18:12Z

src/Standards/Generic/Docs/Commenting/DocCommentStandard.xml

+/**
+ * Short description.
+ *
+ * @param int $foo
+ *
+<em> * @author Some Author
+ * @return int</em>
+ */


As the rule is about the param tags, would it make sense for the  highlighting to wrap the @param tag(s) instead of the non-@param tags ?

I agree and I implemented it in dc22715

jrfnl · 2024-03-26T17:19:09Z

src/Standards/Generic/Docs/Commenting/DocCommentStandard.xml

+ * @deprecated <em>1.0.0</em>
+ * @return     <em>bool</em>


I wonder if the  tags here should include the alignment whitespace ?

That makes sense. I added this change in commit dc22715.

Co-authored-by: Juliette <[email protected]>

rodrigoprimo · 2024-03-27T15:25:07Z

Thanks for your review, @jrfnl. I pushed two commits implementing the changes you suggested, except one that I was not sure about. Could you please take another look?

Based on suggestion during PR review

rodrigoprimo · 2024-04-22T19:11:19Z

@jrfnl, I amended f266412 to use the sniff description that you suggested during our call earlier today. This PR is ready for you to check it again when you have the time. Thanks.

jrfnl

@rodrigoprimo Final dotting of the i's and crossing the t's. Nearly there.

jrfnl · 2024-05-20T15:21:09Z

src/Standards/Generic/Docs/Commenting/DocCommentStandard.xml

+    <![CDATA[
+    Enforces rules related to the formatting of DocBlocks ("Doc Comments") in PHP code.
+
+    DocBlocks are a special type of comment that can provide information about a structural element. In the context of DocBlocks, the following are considered structural elements: require(_once) and include(_once) statements, class, interface, trait, function, property, constant and variable declarations.


Any particular reason not to mention enums ?

Also, would it make more sense to have the (less commonly commented on) require(_once) and include(_once) statements bit at the end ? That would show the structures which more commonly have DocBlocks first.

Good points! I pushed a new commit implementing those two suggestions.

src/Standards/Generic/Docs/Commenting/DocCommentStandard.xml

Co-authored-by: Juliette <[email protected]>

jrfnl

🎉 LGTM! Let's get this puppy merged!

Generic/DocComment: add XML documentation

bb5cd16

github-actions bot added Type: documentation Standard: Generic Status: triage labels Jan 10, 2024

jrfnl mentioned this pull request Jan 13, 2024

Docs: add missing XML documentation for sniffs #148

Open

78 tasks

jrfnl reviewed Feb 23, 2024

View reviewed changes

jrfnl added Status: awaiting feedback and removed Status: triage labels Feb 23, 2024

rodrigoprimo added 2 commits February 28, 2024 16:40

Generic/DocComment: changes to the doc based on PR review

6089ac6

Generic/DocComment: update doc and replace doc comment with DocBlock

5843ebe

rodrigoprimo added 4 commits February 29, 2024 11:53

Group doc for LongNotCapital and ShortNotCapital together

ad2cc5c

Group doc for SpacingBeforeTags, SpacingAfterTagGroup and SpacingBetween

15eb2db

Group doc for ContentAfterOpen and ContentBeforeClose together

1c68da9

Group doc for MissingShort and SpacingBeforeShort together

500f492

jrfnl removed the Status: awaiting feedback label Mar 13, 2024

jrfnl requested changes Mar 26, 2024

View reviewed changes

jrfnl added the Status: awaiting feedback label Mar 26, 2024

Apply suggestions from code review

d592b6f

Co-authored-by: Juliette <[email protected]>

rodrigoprimo force-pushed the documentation-doc-comment branch from 38fc6c7 to d592b6f Compare March 27, 2024 14:05

jrfnl removed the Status: awaiting feedback label Mar 28, 2024

rodrigoprimo added 2 commits April 22, 2024 16:08

Apply remaining suggestions from code review

f266412

Move standard about additional lines before closing tag to end

250effa

Based on suggestion during PR review

rodrigoprimo force-pushed the documentation-doc-comment branch from c0ef1e5 to 250effa Compare April 22, 2024 19:09

jrfnl reviewed May 20, 2024

View reviewed changes

rodrigoprimo and others added 2 commits May 20, 2024 12:31

Further changes to the description

b42cc51

Apply suggestions from code review

80dee19

Co-authored-by: Juliette <[email protected]>

jrfnl approved these changes May 20, 2024

View reviewed changes

jrfnl added this to the 3.10.x Next milestone May 20, 2024

jrfnl merged commit 553b984 into PHPCSStandards:master May 20, 2024
40 checks passed

rodrigoprimo deleted the documentation-doc-comment branch May 20, 2024 16:09

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Generic/DocComment: add XML documentation #247

Generic/DocComment: add XML documentation #247

rodrigoprimo commented Jan 10, 2024

jrfnl left a comment

jrfnl Feb 23, 2024

jrfnl Feb 23, 2024

jrfnl Feb 23, 2024

rodrigoprimo Feb 29, 2024 •

edited

Loading

jrfnl Feb 29, 2024

rodrigoprimo commented Feb 28, 2024 •

edited

Loading

rodrigoprimo commented Feb 29, 2024

jrfnl left a comment

jrfnl Mar 26, 2024

jrfnl Mar 26, 2024

rodrigoprimo Mar 27, 2024

jrfnl Mar 26, 2024

rodrigoprimo Mar 27, 2024

jrfnl Mar 26, 2024

rodrigoprimo Mar 27, 2024

jrfnl Mar 26, 2024

rodrigoprimo Mar 27, 2024

jrfnl Mar 26, 2024

rodrigoprimo Mar 27, 2024

rodrigoprimo commented Mar 27, 2024 •

edited

Loading

rodrigoprimo commented Apr 22, 2024

jrfnl left a comment

jrfnl May 20, 2024

rodrigoprimo May 20, 2024

jrfnl left a comment

	There must be a single blank line after a tag group.
	Tag groups must be separated by a single blank line.

	$error = 'There must be a single blank line after a tag group';
	$fix = $phpcsFile->addFixableError($error, $lastTag, 'SpacingAfterTagGroup');

	Enforces several rules related to the formatting of DocBlocks in PHP code.
	Enforces rules related to the formatting of DocBlocks in PHP code.

	<code title="Valid: The opening and closing DocBlock tags are the only content on the line.">
	<code title="Valid: The opening and closing DocBlock tags have to be on a line by themselves.">

Generic/DocComment: add XML documentation #247

Generic/DocComment: add XML documentation #247

Conversation

rodrigoprimo commented Jan 10, 2024

Description

Suggested changelog entry

Related issues/external references

Types of changes

PR checklist

jrfnl left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

rodrigoprimo Feb 29, 2024 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

rodrigoprimo commented Feb 28, 2024 • edited Loading

rodrigoprimo commented Feb 29, 2024

jrfnl left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

rodrigoprimo commented Mar 27, 2024 • edited Loading

rodrigoprimo commented Apr 22, 2024

jrfnl left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

jrfnl left a comment

Choose a reason for hiding this comment

rodrigoprimo Feb 29, 2024 •

edited

Loading

rodrigoprimo commented Feb 28, 2024 •

edited

Loading

rodrigoprimo commented Mar 27, 2024 •

edited

Loading