Arrays/ArrayDeclarationSpacing: add XML documentation#2682
Arrays/ArrayDeclarationSpacing: add XML documentation#2682dingo-d merged 1 commit intoWordPress:developfrom
Conversation
There was a problem hiding this comment.
Thanks for taking this on @rodrigoprimo !
We've just discussed this PR in a call and while the second standard + code comparison are fine, the first is unclear.
The first block should address the "Arrays with keys containing more than one item, must be declared as multi-line arrays" rule, but the docs as they are, are mixing the first and the second rule for the first <standard> block as well as the first <code_comparison> block.
Want to have another look at this ?
Re: your review comment on a previous iterations of this PR - #2489 (comment) - which calls out that the sniff says the above mentioned rule is about associative arrays, while in reality the sniff just checked that there are keys, not that the keys are strings.
I agree we may need to update the sniff error message (and the docs to match), but that would first need an update to the handbook as this phrasing is taken from there.
In my opinion, that's a minor correction to the handbook which shouldn't need a Make post. The rule has been applied for "arrays with keys" for as long as this sniff exists, not for "associative arrays", so this would just be a case of aligning the handbook with the practiced reality.
|
Thanks for your review, @jrfnl.
You are right that I mixed both rules in the first block. I pushed a new commit with an attempt to clarify things. Please take a look when you get a chance.
Thanks for confirming my suspicion. I opened a PR in the handbook repository suggesting the change: WordPress/wpcs-docs#156. I also prepared a change to the sniff error message that I will pull here if the handbook change is approved. |
jrfnl
left a comment
There was a problem hiding this comment.
Thanks for that update @rodrigoprimo ! LGTM now.
67df3c3 to
b3be7b2
Compare
As suggested by Juliette in WordPress/WordPress-Coding-Standards#2682 (comment), every array has keys, they can be either explicitly declared or implicitly assigned by PHP. Using "explicit keys" makes the wording more precise. Follow-up to 156.
Co-authored-by: RafaelFunchal <RafaelFunchal@users.noreply.github.com> Co-authored-by: mattgaldino <mattgaldino@users.noreply.github.com> Co-authored-by: Rodrigo Primo <rodrigoprimo@users.noreply.github.com> Co-authored-by: Juliette <663378+jrfnl@users.noreply.github.com>
b3be7b2 to
eefd218
Compare
|
Just noting that I squashed the commits in this PR. |
Description
This PR adds XML documentation for the
WordPress.Arrays.ArrayDeclarationSpacingsniff.The documentation is based on the work started by @RafaelFunchal in #2489 and @mattgaldino in #2593. I squashed the original commits and, in a separate commit, made subsequent changes based on code review suggestions.
I suggest squashing those two commits before merging. I'm opening the PR without doing that to make it easier to tell my changes apart from the original changes.
Suggested changelog entry
N/A
Related issues/external references
Related to: #1722
Supersedes: #2489 and #2593
Closes #2489
Closes #2593