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

AlignMultilineCommentFixer - ArrayIndentationFixer - Priority #3885

Conversation

dmvdbrugge
Copy link
Contributor

It should run after ArrayIndentationFixer.

This fixes #3860

It should run after ArrayIndentationFixer.
@dmvdbrugge dmvdbrugge force-pushed the 3860-array_indentation-before-align_multiline_comment branch from a690efc to bd51b51 Compare July 6, 2018 12:12
@julienfalque
Copy link
Member

Note: my commit can be squashed when merging :)

@dmvdbrugge
Copy link
Contributor Author

dmvdbrugge commented Jul 8, 2018

I'm not sure I agree with the added commit, it serves no use other than taking away the focus of why it was added in the first place.

edit: also, the comment is now no longer valid, since the aligning is even more off haha

@julienfalque
Copy link
Member

I added invalid indent on other lines to make array_indentation usage more explicit. This doesn't change the result in any way.

the comment is now no longer valid, since the aligning is even more off haha

What do you mean?

@dmvdbrugge
Copy link
Contributor Author

The comment reads Aligned with the values (to illustrate the point). That no longer holds true because you invalidated the original alignments.

@julienfalque
Copy link
Member

The comment is still aligned with values in the expected result, and never was in the input. Adding invalid indent didn't change that.

@dmvdbrugge
Copy link
Contributor Author

Sorry but no, look again: it's aligned with the keys in the expected result. It was aligned with the values in my original input.

@julienfalque
Copy link
Member

Oh I see what you meant now. But I still prefer the current input: it explicitly shows that indentation of the array changes, while aligning the comment with values (instead of keys) has few value for this test IMO. If we keep current input, the comment should be updated indeed.

@dmvdbrugge
Copy link
Contributor Author

The indentation of the array already changed, the comment gets moved. Or at least the first line of the comment. That is being done by array_indentation and is the exact reason this change is needed, look at the example code in the issue. I prefer to not "overcomplicate" the testcase, and only show the reason for the priority.

@julienfalque julienfalque force-pushed the 3860-array_indentation-before-align_multiline_comment branch from cd444f6 to bd51b51 Compare July 8, 2018 17:41
@julienfalque
Copy link
Member

Ok, I removed my commit.

@SpacePossum
Copy link
Contributor

do we've the same (hidden) priority issue with method_argument_space and method_chaining_indentation?

@SpacePossum SpacePossum added the RTM Ready To Merge label Jul 9, 2018
@SpacePossum SpacePossum changed the title Add priority to AlignMultilineCommentFixer AlignMultilineCommentFixer - ArrayIndentationFixer - Priority Jul 9, 2018
@SpacePossum SpacePossum added this to the 2.12.3 milestone Jul 9, 2018
@dmvdbrugge
Copy link
Contributor Author

dmvdbrugge commented Jul 9, 2018

  • Just tested method_chaining_indentation: it doesn't touch the comment at all (and thus doesn't break alignment).
  • For method_argument_space I fail to see the case sorry.

@SpacePossum
Copy link
Contributor

Thanks @dmvdbrugge.

@SpacePossum SpacePossum merged commit bd51b51 into PHP-CS-Fixer:2.12 Jul 10, 2018
SpacePossum added a commit that referenced this pull request Jul 10, 2018
…ty (dmvdbrugge)

This PR was merged into the 2.12 branch.

Discussion
----------

AlignMultilineCommentFixer - ArrayIndentationFixer - Priority

It should run after ArrayIndentationFixer.

This fixes #3860

Commits
-------

bd51b51 Add priority to AlignMultilineCommentFixer
@SpacePossum SpacePossum removed the RTM Ready To Merge label Jul 10, 2018
@dmvdbrugge dmvdbrugge deleted the 3860-array_indentation-before-align_multiline_comment branch July 30, 2018 19:02
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

4 participants