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.

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

Adding documentation for WordPress.Arrays.ArrayDeclarationSpacing #2489

Open
wants to merge 1 commit into
base: develop
Choose a base branch
from

Conversation

RafaelFunchal
Copy link

Related to #1722

Copy link
Collaborator

@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 working on adding documentation for WordPress.Arrays.ArrayDeclarationSpacing, @RafaelFunchal! I left inline comments suggesting some improvements.

<code title="Valid: There is only one pair of key and value per line.">
<![CDATA[
$args = array(
<em>'post_id' => 22,
Copy link
Collaborator

Choose a reason for hiding this comment

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

In my opinion, it is slightly better if the multi line examples use one pair of <em> tags per line (here and in the valid example below):

$args = array(
    <em>'post_id' => 22,</em>
    <em>'category' => 1</em>
);

This makes a small difference when generating the HTML version of the documentation (vendor/bin/phpcs --generator=HTML --standard=WordPress --sniffs=WordPress.Arrays.ArrayDeclarationSpacing.

Current version:

Screenshot from 2024-09-25 09-41-37

Version with the suggested change:

Screenshot from 2024-09-25 09-41-43

>
<standard>
<![CDATA[
When an array uses associative keys, each pair of key and value must start on a new line.
Copy link
Collaborator

Choose a reason for hiding this comment

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

I know the sniff error message uses the term associative keys, but I'm inclined to think that just keys is better (and we can also consider updating the sniff error message on a separate PR, if the maintainers agree with the point that I'm raising here). Saying associative keys can be misleading as it might mean to some people that indexed arrays that explicitly declare their keys will not trigger this error when they trigger it. What do you think?

);
]]>
</code>
<code title="Invalid: Multiple array items in the same line.">
Copy link
Collaborator

Choose a reason for hiding this comment

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

I wonder if we should include an example of an associative array that triggers this error to make it explicitly that this one is not only about indexed arrays without declared keys?

The error above is only triggered for single-line arrays with declared keys.

Copy link
Collaborator

Choose a reason for hiding this comment

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

Also, should we include in the invalid phrase that this only happens on a multi-line array? Maybe something like: Invalid: More than one item per line in a multi-line array.

>
<standard>
<![CDATA[
When an array uses associative keys, each pair of key and value must start on a new line.
Copy link
Collaborator

Choose a reason for hiding this comment

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

I believe that key/value pair is more commonly used than pair of key and value.

<code_comparison>
<code title="Valid: There is only one pair of key and value per line.">
<![CDATA[
$args = array(
Copy link
Collaborator

Choose a reason for hiding this comment

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

Aside from the issue the sniff is about, please check that the code in each code sample follows the WP Coding Standards. I believe this code example is missing a trailing comma after the last array item, and the alignment of the first double arrow is incorrect.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants