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

Fix handling /** and */ on the same line as the first and/or last annotation #3893

Conversation

dmvdbrugge
Copy link
Contributor

In both Annotation::remove() and PhpdocNoEmptyReturnFixer

This PR is sort-of combined with #3891

@dmvdbrugge dmvdbrugge force-pushed the phpdoc_no_empty_return-dont-break-docblocks branch from f47f675 to e6667b4 Compare July 9, 2018 14:00
@dmvdbrugge dmvdbrugge force-pushed the phpdoc_no_empty_return-dont-break-docblocks branch from e6667b4 to 262e607 Compare July 9, 2018 14:45
@dmvdbrugge dmvdbrugge changed the base branch from 2.2 to 2.12 July 9, 2018 14:45
@dmvdbrugge dmvdbrugge force-pushed the phpdoc_no_empty_return-dont-break-docblocks branch 2 times, most recently from 855f29d to 9b51e8a Compare July 16, 2018 12:16
@dmvdbrugge dmvdbrugge force-pushed the phpdoc_no_empty_return-dont-break-docblocks branch 3 times, most recently from 411bfee to d8a657e Compare July 30, 2018 19:11
@dmvdbrugge dmvdbrugge changed the title Fix handling */ on the same line as the last annotation Fix handling /** and */ on the same line as the first and/or last annotation Oct 16, 2018
@dmvdbrugge dmvdbrugge force-pushed the phpdoc_no_empty_return-dont-break-docblocks branch 2 times, most recently from 6d7f401 to 9e4c786 Compare November 13, 2018 10:02
@keradus
Copy link
Member

keradus commented Nov 20, 2018

@dmvdbrugge , @kubawerlos ,

are this and #4035 PRs handling the very same issue? how shall we proceed on those two ?

@dmvdbrugge
Copy link
Contributor Author

Seems to me like they could live together, they don't even touch the same files. They also solve a problem that looks the same but is not entirely equal, even though it could result in the same outcome.

My suggestion would be just merge both.

@kubawerlos
Copy link
Contributor

I'd like them both to be merged as well.

Reading them again it got me thinking why Line::isTheStart checks if false !== strpos($this->content, '/**');? Wouldn't make more sense to check if it is 0?

@dmvdbrugge
Copy link
Contributor Author

Wouldn't make more sense to check if it is 0?

Can't because of whitespace, I think. Could make it a regex though.

I hadn't even looked into the Line class itself, but now you caused me to 😉and I see that containsUsefulContent excludes start and end lines, which it should not (because as we both discovered, start and end lines actually could contain useful content). Shall I fix that in this PR too?

… annotation

In both `Annotation::remove()` and `PhpdocNoEmptyReturnFixer`
@julienfalque julienfalque force-pushed the phpdoc_no_empty_return-dont-break-docblocks branch from cfe8568 to ad23563 Compare April 11, 2020 09:16
@julienfalque
Copy link
Member

@dmvdbrugge I allowed myself to rebase your work to fix a conflict. I also rewrote test cases in AnnotationTest to make them easier to read. For me this can be merged now.

@julienfalque julienfalque added this to the 2.15.7 milestone Apr 11, 2020
@keradus keradus modified the milestones: 2.15.7, 2.15.8 Apr 15, 2020
@julienfalque julienfalque added the RTM Ready To Merge label Apr 17, 2020
@SpacePossum
Copy link
Contributor

Thank you @dmvdbrugge.

@SpacePossum SpacePossum merged commit d945442 into PHP-CS-Fixer:2.15 Apr 19, 2020
@SpacePossum SpacePossum removed the RTM Ready To Merge label Apr 19, 2020
@dmvdbrugge dmvdbrugge deleted the phpdoc_no_empty_return-dont-break-docblocks branch April 26, 2020 19:42
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

5 participants