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.

Sign up for GitHub

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

Generic/DocComment: add XML documentation #247

Merged
merged 12 commits into from
May 20, 2024
Merged

Generic/DocComment: add XML documentation #247

Changes from 7 commits
Commits
Show all changes
12 commits
Select commit Hold shift + click to select a range
bb5cd16
Generic/DocComment: add XML documentation
rodrigoprimo Jan 10, 2024
6089ac6
Generic/DocComment: changes to the doc based on PR review
rodrigoprimo Feb 28, 2024
5843ebe
Generic/DocComment: update doc and replace doc comment with DocBlock
rodrigoprimo Feb 28, 2024
ad2cc5c
Group doc for LongNotCapital and ShortNotCapital together
rodrigoprimo Feb 29, 2024
15eb2db
Group doc for SpacingBeforeTags, SpacingAfterTagGroup and SpacingBetween
rodrigoprimo Feb 29, 2024
1c68da9
Group doc for ContentAfterOpen and ContentBeforeClose together
rodrigoprimo Feb 29, 2024
500f492
Group doc for MissingShort and SpacingBeforeShort together
rodrigoprimo Feb 29, 2024
d592b6f
Apply suggestions from code review
rodrigoprimo Mar 27, 2024
f266412
Apply remaining suggestions from code review
rodrigoprimo Mar 27, 2024
250effa
Move standard about additional lines before closing tag to end
rodrigoprimo Mar 28, 2024
b42cc51
Further changes to the description
rodrigoprimo May 20, 2024
80dee19
Apply suggestions from code review
rodrigoprimo May 20, 2024
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
268 changes: 268 additions & 0 deletions src/Standards/Generic/Docs/Commenting/DocCommentStandard.xml
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -0,0 +1,268 @@
<documentation title="Doc Comment">
<standard>
<![CDATA[
Enforces several rules related to the formatting of DocBlocks in PHP code.
Copy link
Member

Choose a reason for hiding this comment

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

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.

Copy link
Member

Choose a reason for hiding this comment

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

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

Copy link
Contributor Author

Choose a reason for hiding this comment

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

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.

]]>
</standard>
<standard>
<![CDATA[
DocBlock must not be empty.
]]>
</standard>
<code_comparison>
<code title="Valid: DocBlock with some content.">
<![CDATA[
/**
* <em>Some content.</em>
*/
]]>
</code>
<code title="Invalid: Empty DocBlock.">
<![CDATA[
/**
* <em></em>
*/
]]>
</code>
</code_comparison>
<standard>
<![CDATA[
The opening and closing DocBlock tags must be the only content on the line.
]]>
</standard>
<code_comparison>
<code title="Valid: The opening and closing DocBlock tags are the only content on the line.">
Copy link
Member

Choose a reason for hiding this comment

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

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

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Implemented in dc22715

<![CDATA[
<em>/**</em>
* Short description.
<em>*/</em>
]]>
</code>
<code title="Invalid: The opening and closing DocBlock tags are not the only content on the line.">
<![CDATA[
<em>/** Short description.</em>
*/

/**
<em>* Short description */</em>
]]>
</code>
</code_comparison>
<standard>
<![CDATA[
There must be no additional blank lines before the closing DocBlock tag.
]]>
</standard>
<code_comparison>
<code title="Valid: no additional blank lines before the closing DocBlock tag.">
<![CDATA[
/**
* Short description.
<em></em> */
]]>
</code>
<code title="Invalid: additional blank lines before the closing DocBlock tag.">
<![CDATA[
/**
* Short description.
<em> *</em>
*/
]]>
</code>
</code_comparison>
<standard>
<![CDATA[
The DocBlock must have a short description, and it must be on the first line.
]]>
</standard>
<code_comparison>
<code title="Valid: DocBlock with a short description on the first line.">
<![CDATA[
/**
* <em>Short description.</em>
*/
]]>
</code>
<code title="Invalid: DocBlock without a short description or short description not on the first line.">
<![CDATA[
/**
* <em></em>@return int
*/

/**
<em> *</em>
* Short description.
*/
]]>
</code>
</code_comparison>
<standard>
<![CDATA[
The short and long descriptions must start with a capital letter.
]]>
</standard>
<code_comparison>
<code title="Valid: Short and long descriptions start with a capital letter.">
<![CDATA[
/**
* <em>S</em>hort description.
*
* <em>L</em>ong description.
*/
]]>
</code>
<code title="Invalid: Short or long descriptions don't start with a capital letter.">
<![CDATA[
/**
* <em>s</em>hort description.
*
* <em>l</em>ong description.
*/
]]>
</code>
</code_comparison>
<standard>
<![CDATA[
There must be exactly one blank line separating the short description, the long description and tag groups.
]]>
</standard>
<code_comparison>
<code title="Valid: One blank line separating the short description, the long description and tag groups.">
<![CDATA[
/**
* Short description.
<em> *<em>
* Long description.
<em> *</em>
* @param int $foo
*/
]]>
</code>
<code title="Invalid: More than one or no blank line separating the short description, the long description and tag groups.">
<![CDATA[
/**
* Short description.
<em> *
*
</em>
* Long description.
* @param int $foo
*/
]]>
</code>
</code_comparison>
<standard>
<![CDATA[
Parameter tags must be grouped together.
]]>
</standard>
<code_comparison>
<code title="Valid: Parameter tags grouped together.">
<![CDATA[
/**
* Short description.
*
<em> * @param int $foo
* @param string $bar</em>
*/
]]>
</code>
<code title="Invalid: Parameter tags not grouped together.">
<![CDATA[
/**
* Short description.
*
* @param int $foo
<em> *</em>
* @param string $bar
*/
]]>
</code>
</code_comparison>
<standard>
<![CDATA[
Parameter tags must not be grouped together with other tags.
]]>
</standard>
<code_comparison>
<code title="Valid: Parameter tags are not grouped together with other tags.">
<![CDATA[
/**
* Short description.
*
* @param int $foo
*
<em> * @author Some Author
* @return int</em>
*/
Copy link
Member

Choose a reason for hiding this comment

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

While valid, the @author tag is a bit dated and not very commonly used. What about using the @since tag instead in the example ? I'm thinking something like this:

Suggested change
/**
* Short description.
*
* @param int $foo
*
<em> * @author Some Author
* @return int</em>
*/
/**
* Short description.
*
<em> * @param int $foo
*
* @return int
*
* @since 5.4.8</em>
*/

Copy link
Member

Choose a reason for hiding this comment

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

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

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I agree and I implemented it in dc22715

]]>
</code>
<code title="Invalid: Parameter tags grouped together with other tags.">
<![CDATA[
/**
* Short description.
*
<em> * @param int $foo
* @author Some Author
* @return int</em>
*/
]]>
</code>
</code_comparison>
<standard>
<![CDATA[
Tag values for different tags in the same group must be aligned with each other.
]]>
</standard>
<code_comparison>
<code title="Valid: Tag values for different tags in the same tag group are aligned with each other.">
<![CDATA[
/**
* Short description.
*
* @deprecated <em>1.0.0</em>
* @return <em>bool</em>
Copy link
Member

Choose a reason for hiding this comment

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

I wonder if the <em> tags here should include the alignment whitespace ?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

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

*/
Copy link
Member

Choose a reason for hiding this comment

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

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 <em>1.0.0</em>
* @return <em>bool</em>
*/
/**
* Short description.
*
* @since <em>0.5.3</em>
* @deprecated <em>1.0.0</em>
*/

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Also implemented in dc22715

]]>
</code>
<code title="Invalid: Tag values for different tags in the same tag group are not aligned with each other.">
<![CDATA[
/**
* Short description.
*
* @deprecated <em>1.0.0</em>
* @return <em>bool</em>
*/
]]>
</code>
</code_comparison>
<standard>
<![CDATA[
Parameter tags must be defined before other tags in a DocBlock.
]]>
</standard>
<code_comparison>
<code title="Valid: Parameter tags are defined first.">
<![CDATA[
/**
* Short description.
*
* <em>@param string $foo</em>
*
* @return void
*/
]]>
</code>
<code title="Invalid: Parameter tags are not defined first.">
<![CDATA[
/**
* Short description.
*
* @return void
*
* <em>@param string $bar</em>
*/
]]>
</code>
</code_comparison>
</documentation>
Loading