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

Docs/WordPress.Arrays.ArrayIndentation #1744

Merged
merged 6 commits into from
Oct 13, 2019

Conversation

Mike-Hermans
Copy link
Contributor

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

Related to #1722

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

Once fixed, please rebase locally, and squash all the commits down to a single commit, and force-push that back to your same branch - that will keep the git history clean. Thanks!

WordPress/Docs/Arrays/ArrayIndentationStandard.xml Outdated Show resolved Hide resolved
@Mike-Hermans
Copy link
Contributor Author

@GaryJones Something like this?

@GaryJones
Copy link
Member

Something like this?

@Mike-Hermans Perfect!

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, mostly looking very good.

Sorry it took me a while to get to this PR. I've left some remarks in-line.

Additionally, the sniff also checks the indentation of multi-line array items and that is - so far - not yet covered in the documentation.

What I mean by that is for instance:

$a = array(
    'phrase' => 'start of phrase'
        . 'concatented additional phrase'
        . 'more text',
);

The rule here is that the subsequent lines should be indented at least as much as the first line of the array item.

Lastly, the sniff will ignore subsequent lines in multi-line heredoc/nowdocs for indentation, but will check the opener of the heredoc/nowdoc and the , at the end of that array item for correct indentation.
Think:

$a = array(
    <<<EOD
Some text
Some more text
EOD
    ,
);

It would be awesome if you would be willing and able to update the documentation to reflect this. Thanks!

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

@jrfnl Sorry for the late update.

I've updated the documentation based on your feedback. I think the heredoc/nowdoc documentation may be a bit too verbose, but I didn't know a more compact way to explain it. Could you take a look at it?

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

Sorry for the late update.

No worries, happy to see the update now!

I've updated the documentation based on your feedback. I think the heredoc/nowdoc documentation may be a bit too verbose, but I didn't know a more compact way to explain it. Could you take a look at it?

Ok, I've had a look and I may have an idea which could work.

I think it could both be covered by the standard text for multi-line array items. That standard would then have two examples, either as separate blocks (two code comparisons) or with both code samples in one block.

Suggestion for the adjusted text for the multi-line array item standard to cover both (and to make it crystal clear this applies to subsequent lines only):

Subsequent lines in multiline array items should be indented at least as much as the first line of the array item.
For heredocs/nowdocs, this does not apply to the content of the heredoc/nowdoc or the closer, but it does apply to the comma separating the item from the next.

What do you think ?

'phrase' => 'start of phrase'
. 'concatented additional phrase'
. 'more text',
);
Copy link
Member

Choose a reason for hiding this comment

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

Eh.. this example is actually valid. Per the standard "indented at least as much". So indented the same is perfectly fine, indented more as well, indented less: that's the no-no. 😄

@Mike-Hermans
Copy link
Contributor Author

I've made the requested changes. However I've noticed when displaying the standard in the terminal that with the last code example, 'Invalid: Opener is aligned incorrectly to match the closer. The comma does not align correctly with the array indentation.', 'correctly' and 'with' are displayed together as 'correctlywith' even though there is a space in the xml file. What could be the cause of this?

@jrfnl
Copy link
Member

jrfnl commented Oct 13, 2019

However I've noticed when displaying the standard in the terminal that with the last code example, 'Invalid: Opener is aligned incorrectly to match the closer. The comma does not align correctly with the array indentation.', 'correctly' and 'with' are displayed together as 'correctlywith' even though there is a space in the xml file. What could be the cause of this?

That was bug in PHPCS upstream and has been fixed in PHPCS 3.5.0 which was released a couple of weeks ago.
See: squizlabs/PHP_CodeSniffer#2536

@jrfnl jrfnl added this to the 2.2.0 milestone Oct 13, 2019
@jrfnl
Copy link
Member

jrfnl commented Oct 13, 2019

Thanks for the update @Mike-Hermans !

@jrfnl jrfnl merged commit e38cee8 into WordPress:develop Oct 13, 2019
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