Repository navigation
[Update] Docs for array declaration spacing sniff - #2593
matt-galdino wants to merge 5 commits into
Conversation
Update terminology from 'pair of key and value' to 'key/value pair'
- Add trailing commas after last array items - Align double arrows for better readability
rodrigoprimo
left a comment
There was a problem hiding this comment.
Thanks for working on this PR @matt-galdino!
I left a comment about one of my original remarks and a few other things that I had missed in my original review.
|
|
||
| Associative arrays with multiple key-value pairs must also follow this rule. | ||
| ]]> | ||
|
|
||
|
||
| $settings = array( | ||
| 'width' => 300, | ||
| 'height' => 200, | ||
| 'color' => 'blue', | ||
| ); | ||
| ]]> | ||
|
||
| $settings = array( | ||
| 'width' => 300, 'height' => 200, | ||
| 'color' => 'blue', | ||
| ); | ||
| ]]> | ||
There was a problem hiding this comment.
I'm sorry if my comment in the original PR was not clear, but in https://github.com/WordPress/WordPress-Coding-Standards/pull/2489/files#r1775277204, I was not suggesting to add a new block. I don't think this is necessary. Instead, I was suggesting adding a new valid/invalid example to the block that already exists. blocks can have multiple examples when needed. Or do you see a reason to have a separate block in this case?
| $args = array( | ||
| 'post_id' => 22, 'category' => 1, | ||
| ); |
There was a problem hiding this comment.
One thing that I missed in my original review is that in order for this example to trigger the AssociativeArrayFound error, which I believe is the intention of this example, everything should be in the same line. Currently, this example triggers the ArrayItemNoNewLine error that is already covered in the block below. Could you please look into that? Let me know if you need any help. You might need to adjust the example so that it fits in a single line because of the 48 characters per line limit (not counting the tags).
| > | ||
|
|
||
| When an array uses keys, each key/value pair must start on a new line. |
There was a problem hiding this comment.
There is something else that I missed in my original review. Using the default configuration of the sniff, which is what we want to document the XML file, the error ArrayItemNoNewLine is only triggered for multi-item single-line arrays, so it might be worth including this information in the description and in the code comparison titles.
| When an array uses keys, each key/value pair must start on a new line. | |
| When a multi-item array uses keys, each key/value pair must start on a new line. |
| ]]> | ||
|
|
||
|
There was a problem hiding this comment.
| <code title="Valid: There is only one key/value pair per line."> | |
| <code title="Valid: Only one key/value pair per line on a multi-item array."> |
| ); | ||
| ]]> | ||
|
There was a problem hiding this comment.
| <code title="Invalid: More than one key/value pair per line."> | |
| <code title="Invalid: Single line multi-item array using keys."> |
|
Thank you, @rodrigoprimo! I'm willing to work on the updates you suggested very soon. |
|
Sounds good. Thanks, @matt-galdino. |
|
@matt-galdino, I was just wondering if you'll have a chance to finish this off in the near future. It would be great if this PR could be included in the next WPCS release. If you haven't got time or lost interest, please let us know and we'll see if we can find someone to take over. Thanks! |
Related to PR #1722
Surpasses RafaelFunchal's changes made here: #2489
Closes #2489