Generic/ArrayIndent: add XML documentation #432

rodrigoprimo · 2024-04-04T15:52:26Z

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.

rodrigoprimo · 2024-04-04T15:54:54Z

src/Standards/Generic/Docs/Arrays/ArrayIndentStandard.xml

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


I opted to use opening square bracket/parenthesis here and in a few other places as the sniff supports both array syntaxes. That is not super correct as in the old syntax, the opening token of an array is not just a parenthesis, but actually array(.

What about using the phrase "open brace" (and "close brace" for the end, of course) ? Would that work ? What do you think ?

Thanks for your suggestion. I just implemented it, but I ended up using "opening/closing brace" instead of "open/close brace" as the former is present in the PHP documentation (https://www.php.net/manual/en/control-structures.alternative-syntax.php), while the latter is not.

src/Standards/Generic/Docs/Arrays/ArrayIndentStandard.xml

jrfnl

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

jrfnl · 2024-05-20T15:52:18Z

src/Standards/Generic/Docs/Arrays/ArrayIndentStandard.xml

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


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.

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

jrfnl · 2024-09-30T15:40:32Z

src/Standards/Generic/Docs/Arrays/ArrayIndentStandard.xml

+    $a =
+<em>    [</em>
+        1,
+        2,
+    ];


What about adding a second code sample here showing that having the opening brace on the same line as the assignment is also perfectly valid under this rule ?

jrfnl · 2024-09-30T15:44:47Z

src/Standards/Generic/Docs/Arrays/ArrayIndentStandard.xml

+    </code_comparison>
+    <standard>
+    <![CDATA[
+    The elements of an array must be indented four spaces beyond the indentation level of the opening square bracket/parenthesis.


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.

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 · 2024-09-30T15:52:33Z

src/Standards/Generic/Docs/Arrays/ArrayIndentStandard.xml

+    </code_comparison>
+    <standard>
+    <![CDATA[
+    The closing square bracket/parenthesis must be indented four spaces less than the array elements.


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) ?

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.

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

rodrigoprimo

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

rodrigoprimo · 2024-10-10T12:09:53Z

src/Standards/Generic/Docs/Arrays/ArrayIndentStandard.xml

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


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

rodrigoprimo · 2024-10-10T12:10:09Z

src/Standards/Generic/Docs/Arrays/ArrayIndentStandard.xml

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


Thanks for your suggestion. I just implemented it, but I ended up using "opening/closing brace" instead of "open/close brace" as the former is present in the PHP documentation (https://www.php.net/manual/en/control-structures.alternative-syntax.php), while the latter is not.

rodrigoprimo · 2024-10-10T12:22:28Z

src/Standards/Generic/Docs/Arrays/ArrayIndentStandard.xml

+    $a =
+<em>    [</em>
+        1,
+        2,
+    ];


rodrigoprimo · 2024-10-10T12:29:03Z

src/Standards/Generic/Docs/Arrays/ArrayIndentStandard.xml

+    </code_comparison>
+    <standard>
+    <![CDATA[
+    The closing square bracket/parenthesis must be indented four spaces less than the array elements.


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 · 2024-10-10T18:28:55Z

src/Standards/Generic/Docs/Arrays/ArrayIndentStandard.xml

+    </code_comparison>
+    <standard>
+    <![CDATA[
+    The elements of an array must be indented four spaces beyond the indentation level of the opening square bracket/parenthesis.


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?

Generic/ArrayIndent: add XML documentation

948b2fb

github-actions bot added Type: documentation Standard: Generic Status: triage labels Apr 4, 2024

rodrigoprimo commented Apr 4, 2024

View reviewed changes

src/Standards/Generic/Docs/Arrays/ArrayIndentStandard.xml Show resolved Hide resolved

rodrigoprimo commented Apr 4, 2024

View reviewed changes

src/Standards/Generic/Docs/Arrays/ArrayIndentStandard.xml Outdated Show resolved Hide resolved

jrfnl mentioned this pull request May 16, 2024

Docs: add missing XML documentation for sniffs #148

Open

78 tasks

jrfnl requested changes Sep 30, 2024

View reviewed changes

jrfnl added the Status: awaiting feedback label Sep 30, 2024

rodrigoprimo and others added 2 commits October 10, 2024 09:08

Apply suggestions from code review

498983d

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

Implement code review suggestions

e4fb292

rodrigoprimo commented Oct 10, 2024

View reviewed changes

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Generic/ArrayIndent: add XML documentation #432

Generic/ArrayIndent: add XML documentation #432

rodrigoprimo commented Apr 4, 2024

rodrigoprimo Apr 4, 2024

jrfnl May 20, 2024

rodrigoprimo Oct 10, 2024

jrfnl left a comment

jrfnl May 20, 2024

rodrigoprimo Oct 10, 2024

jrfnl Sep 30, 2024

rodrigoprimo Oct 10, 2024

jrfnl Sep 30, 2024

rodrigoprimo Oct 10, 2024

jrfnl Sep 30, 2024

rodrigoprimo Oct 10, 2024

rodrigoprimo left a comment

rodrigoprimo Oct 10, 2024

rodrigoprimo Oct 10, 2024

rodrigoprimo Oct 10, 2024

rodrigoprimo Oct 10, 2024

rodrigoprimo Oct 10, 2024

	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.

Generic/ArrayIndent: add XML documentation #432

Are you sure you want to change the base?

Generic/ArrayIndent: add XML documentation #432

Conversation

rodrigoprimo commented Apr 4, 2024

Description

Suggested changelog entry

Related issues/external references

Types of changes

PR checklist

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

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