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

Array sniffs documentation (Related to #1722) #1737

Merged
merged 6 commits into from Oct 13, 2019

Conversation

Mike-Hermans
Copy link
Contributor

@Mike-Hermans Mike-Hermans commented Jun 20, 2019

Adds documentation for the following sniffs:

  • Arrays.MultipleStatementAlignment
  • Arrays.ArrayKeySpacingRestrictions

Related to #1722

]]>
</standard>
<code_comparison>
<code title="Correct whitespace for index keys">
Copy link
Member

Choose a reason for hiding this comment

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

Start with Valid: .

$post_title = $post['post_title'];
]]>
</code>
<code title="Incorrect spacing">
Copy link
Member

Choose a reason for hiding this comment

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

Start with Invalid: .

]]>
</standard>
<code_comparison>
<code title="Double arrow operators aligned">
Copy link
Member

Choose a reason for hiding this comment

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

Start with Valid: .

);
]]>
</code>
<code title="Not aligned; harder to read">
Copy link
Member

Choose a reason for hiding this comment

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

Start with Invalid: .

XML Documentation automation moved this from In progress to Review in progress Jun 23, 2019
XML Documentation automation moved this from Review in progress to Reviewer approved Jun 25, 2019
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.

Looks good to me.

Can you please rebase locally to squash the commits into a single commit (or better: one per XML file), and then force-push please?

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.

@Mike-Hermans Hi Mike, please accept my apologies for the delay in me reviewing this PR and thank you for the adjustments you've made so far.

The docs are mostly looking very good, just a few more remarks.

ArrayKeySpacingRestrictions

  • The docs are missing the <em> ...</em> tags to highlight good/bad code, but we weren't that clear about that in the initial explanation, so sorry about that!.
  • Would it have value to add an example of an array key consisting of a PHP constant ? and/or an array key which is concatenated together ? In both cases, spaces on the inside of the brackets are expected.

MultipleStatementAlignment

  • I'm missing a code sample where there are no spaces/inconsistent spaces around the arrows. The current code sample only addresses aligning the arrows.
    Think: too much space after the arrow, no space before/after the arrow.
  • Also: What about adding another example with a single line array ?

XML Documentation automation moved this from Reviewer approved to Review in progress Sep 11, 2019
@jrfnl
Copy link
Member

jrfnl commented Oct 7, 2019

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

@Mike-Hermans
Copy link
Contributor Author

I've updated the examples in ArrayKeySpacingRestrictions to include both a concatenated string and a constant.

For MultipleStatementAlignment I've added another block with code comparision to show the correct spacing in single line arrays, and the multiple spaces for alignment in multi-line arrays.

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 @Mike-Hermans, thank you for the update. Nearly there.

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.

@Mike-Hermans Thanks for that last update. Merging now.

XML Documentation automation moved this from Review in progress to Reviewer approved Oct 13, 2019
@jrfnl jrfnl added this to the 2.2.0 milestone Oct 13, 2019
@jrfnl jrfnl merged commit 058be1b into WordPress:develop Oct 13, 2019
XML Documentation automation moved this from Reviewer approved to Done Oct 13, 2019
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
Development

Successfully merging this pull request may close these issues.

None yet

4 participants