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

Add the documentation for WordPress.Arrays.CommaAfterArrayItem #1734

Conversation

marconmartins
Copy link
Contributor

@marconmartins marconmartins commented Jun 20, 2019

Related #1722

Copy link
Member

@GaryJones GaryJones left a comment

Choose a reason for hiding this comment

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

I started making a note of all of the em tags instances that need fixing, but there are more.

WordPress/Docs/Arrays/CommaAfterArrayItemStandard.xml Outdated Show resolved Hide resolved
WordPress/Docs/Arrays/CommaAfterArrayItemStandard.xml Outdated Show resolved Hide resolved
WordPress/Docs/Arrays/CommaAfterArrayItemStandard.xml Outdated Show resolved Hide resolved
WordPress/Docs/Arrays/CommaAfterArrayItemStandard.xml Outdated Show resolved Hide resolved
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.

Hi @marconmartins Thank you so much for this PR.

The code examples chosen look good, though similar to what @GaryJones says above, the fine details can use a little improvement still.

More than anything, when I look at this documentation, it feels to me that more <standard> blocks are needed. Basically one for each error thrown. Similar to the example linked in the original issue.

So, may I suggest the following structure for the document ?

  • Standard: Each item in a multi-line array should be followed by a comma.
    • Code comparison 2 and 3 of the current docs.
      You may not even need both as, for this sniff, it makes no difference whether it is a keyed/associative array or not, only that it is multi-line, so just having the keyed example here should probably be enough.
  • Standard: There should be no comma after the last item in a single-line array.
    • Code comparison 1 of the current docs.
  • Standard: There should be no space between the comma and the end of the array item.
    • Code comparison 4 of the current docs.
  • Standard: There should be exactly one space between the comma and the start of the next array item for single-line items.
    • Code comparison 5 of the current docs.

Other than that:

  • When the sniff checks for exactly one or no space, it would probably be helpful to use that exact wording in the "Valid"/"Invalid" titles. The term "spacing" could be considered a bit too generic and could be confusing to people.
  • The <em> ... </em> tags should be around the issue the code comparison is demonstrating - most of the time, this will be around the comma, with or without whitespace -, not around the array open/close tags, as those are not the ones being checked by this sniff.

I hope this helps and I look forward to seeing the next iteration of these docs.

WordPress/Docs/Arrays/CommaAfterArrayItemStandard.xml Outdated Show resolved Hide resolved
WordPress/Docs/Arrays/CommaAfterArrayItemStandard.xml Outdated Show resolved Hide resolved
WordPress/Docs/Arrays/CommaAfterArrayItemStandard.xml Outdated Show resolved Hide resolved
WordPress/Docs/Arrays/CommaAfterArrayItemStandard.xml Outdated Show resolved Hide resolved
WordPress/Docs/Arrays/CommaAfterArrayItemStandard.xml Outdated Show resolved Hide resolved
WordPress/Docs/Arrays/CommaAfterArrayItemStandard.xml Outdated Show resolved Hide resolved
WordPress/Docs/Arrays/CommaAfterArrayItemStandard.xml Outdated Show resolved Hide resolved
WordPress/Docs/Arrays/CommaAfterArrayItemStandard.xml Outdated Show resolved Hide resolved
@jrfnl
Copy link
Member

jrfnl commented Oct 7, 2019

@marconmartins Just wondering if you'll have a chance to finish this off in the near future.
I'd like to release WPCS 2.2.0 soon and it would be great if this PR could be included.

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.

marconmartins and others added 2 commits May 16, 2022 15:35
* Correct placement of the `<em>` tags.
* Correct indentation.
* Split the docs into multiple standards with their own description and group the valid/invalid code samples with the appropriate standard.
* Make the valid/invalid descriptions more precise.
* Use long arrays instead of short arrays in the code samples, so the code samples only violate the WPCS rules for the rule being documented.
@jrfnl jrfnl force-pushed the docs/WordPress_Arrays_CommaAfterArrayItem branch from aecb113 to 7c37ddd Compare May 16, 2022 14:17
@jrfnl
Copy link
Member

jrfnl commented May 16, 2022

I've rebased the PR and added one commit to make the fixes needed. Let's get this merged!

For whomever will merge this: probably squash/merge would be a good idea.

@jrfnl jrfnl added this to the 3.0.0 milestone May 16, 2022
@dingo-d dingo-d requested a review from GaryJones May 16, 2022 15:52
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.

4 participants