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

Jump to bottom

Generic/ArrayIndent: add XML documentation #432

Merged
merged 4 commits into from
Oct 31, 2024
Merged

Generic/ArrayIndent: add XML documentation #432

merged 4 commits into from
Oct 31, 2024

Conversation

rodrigoprimo
Copy link
Contributor

Description

This PR adds the XML documentation for the Generic.Arrays.ArrayIndent sniff.

I left some inline comments below highlighting decisions I made while creating this documentation that I'm not sure about.

Suggested changelog entry

Add XML documentation for the Generic.Arrays.ArrayIndent 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 jrfnl mentioned this pull request May 16, 2024
Open
77 tasks
jrfnl
jrfnl requested changes Sep 30, 2024
View reviewed changes
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.

@rodrigoprimo Sorry for my tardiness with this review.

Generally speaking the documentation is good, there is just some phrasing which could be misinterpreted/interpreted in different ways, which may be confusing . Let's have another look at how to make it as clear as possible.

I think with some small tweaks this can probably be finished quickly. Again, my apologies for the delay in the review.

src/Standards/Generic/Docs/Arrays/ArrayIndentStandard.xml Outdated
<documentation title="Array Indent">
<standard>
<![CDATA[
The opening square bracket/parenthesis of a multi-line array must be indented at least to the same level of the start of the statement.
Copy link
Member

Choose a reason for hiding this comment

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

As a possible alternative (to get away from the kind of braces):

Suggested change
The opening square bracket/parenthesis of a multi-line array must be indented at least to the same level of the start of the statement.
The start of a multi-line array must be indented at least to the same level as the start of the statement.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

IMO, "opening/closing brace" is clearer than "start/end" so I'm inclined to use that. Please let me know if you disagree.

src/Standards/Generic/Docs/Arrays/ArrayIndentStandard.xml Outdated
</code_comparison>
<standard>
<![CDATA[
The elements of an array must be indented four spaces beyond the indentation level of the opening square bracket/parenthesis.
Copy link
Member

Choose a reason for hiding this comment

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

Okay, not sure how to rephrase this, but there are some things here which don't ring true/could be confusing:

  • The indentation is only checked for the first line of each element (subsequent lines are not checked for indentation).
  • "Four spaces beyond"- not sure how much margin that give to interpret this as "at least four spaces", while the rule is "exactly four spaces".
  • "the indentation level of the opening square bracket/parenthesis" - it's not about the indentation/position of the brackets, but about the start of statement which includes the array opener.

Let's rethink the phrasing of this rule.

Note: this also means the Valid/Invalid phrasing may need updating.

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 went with "The first line of each array element must be indented exactly four spaces more than the start of the statement". I also updated the valid/invalid sentences accordingly and changed the <em> tags to highlight only the spaces and not the array element. What do you think?

src/Standards/Generic/Docs/Arrays/ArrayIndentStandard.xml Outdated
</code_comparison>
<standard>
<![CDATA[
The closing square bracket/parenthesis must be indented four spaces less than the array elements.
Copy link
Member

Choose a reason for hiding this comment

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

Would this be clearer if the indentation was phrased to be related to "same level of indentation as the start of the statement containing the array opener" (or something along those lines) ?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

To be honest, I don't have a strong preference, but I went with your suggestion anyway because you have more experience with this than I do.

rodrigoprimo
rodrigoprimo commented Oct 10, 2024
View reviewed changes
Copy link
Contributor Author

@rodrigoprimo rodrigoprimo left a comment

Choose a reason for hiding this comment

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

Thanks for your review, @jrfnl. I believe I addressed all the points that you raised. Could you please take a second look?

src/Standards/Generic/Docs/Arrays/ArrayIndentStandard.xml Outdated
<documentation title="Array Indent">
<standard>
<![CDATA[
The opening square bracket/parenthesis of a multi-line array must be indented at least to the same level of the start of the statement.
Copy link
Contributor Author

Choose a reason for hiding this comment

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

IMO, "opening/closing brace" is clearer than "start/end" so I'm inclined to use that. Please let me know if you disagree.

src/Standards/Generic/Docs/Arrays/ArrayIndentStandard.xml Outdated
</code_comparison>
<standard>
<![CDATA[
The closing square bracket/parenthesis must be indented four spaces less than the array elements.
Copy link
Contributor Author

Choose a reason for hiding this comment

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

To be honest, I don't have a strong preference, but I went with your suggestion anyway because you have more experience with this than I do.

src/Standards/Generic/Docs/Arrays/ArrayIndentStandard.xml Outdated
</code_comparison>
<standard>
<![CDATA[
The elements of an array must be indented four spaces beyond the indentation level of the opening square bracket/parenthesis.
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 went with "The first line of each array element must be indented exactly four spaces more than the start of the statement". I also updated the valid/invalid sentences accordingly and changed the <em> tags to highlight only the spaces and not the array element. What do you think?

@jrfnl
Copy link
Member

jrfnl commented Oct 31, 2024

I've made some final tweaks to the docs after discussing these with @rodrigoprimo off-GH. Merging once the build has passed.

@jrfnl jrfnl added this to the 3.11.0 milestone Oct 31, 2024
@jrfnl jrfnl merged commit 0b81673 into PHPCSStandards:master Oct 31, 2024
41 checks passed
@jrfnl jrfnl deleted the documentation-array-indent branch October 31, 2024 12:44
jrfnl pushed a commit that referenced this pull request Oct 31, 2024
@rodrigoprimo @jrfnl
Generic/ArrayIndent: add XML documentation (#432)
5a745e8
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@jrfnl jrfnl jrfnl approved these changes

Assignees
No one assigned
Labels
Standard: Generic Type: documentation
Projects
None yet
Milestone
3.11.0
Development

Successfully merging this pull request may close these issues.
2 participants
@rodrigoprimo @jrfnl