-
-
Notifications
You must be signed in to change notification settings - Fork 490
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
Docs/WordPress.Arrays.ArrayIndentation #1744
Conversation
There was a problem hiding this 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!
cf11006
to
8c065c4
Compare
@GaryJones Something like this? |
@Mike-Hermans Perfect! |
There was a problem hiding this 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!
@Mike-Hermans Just wondering if you'll have a chance to finish this off in the near future. 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. |
@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? |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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', | ||
); |
There was a problem hiding this comment.
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. 😄
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? |
That was bug in PHPCS upstream and has been fixed in PHPCS 3.5.0 which was released a couple of weeks ago. |
Thanks for the update @Mike-Hermans ! |
Related to #1722