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

PhpdocAlignFixer: fix property-read/property-write descriptions not getting aligned #6174

Merged
merged 1 commit into from Dec 15, 2021
Merged

PhpdocAlignFixer: fix property-read/property-write descriptions not getting aligned #6174

merged 1 commit into from Dec 15, 2021

Conversation

antichris
Copy link
Contributor

@antichris antichris commented Dec 14, 2021

Although the PHPDoc descriptions of @property tags did get aligned vertically, the @property-read and @property-write ones didn't. Now they do.

This is done by adding @property-read and @property-write to the TAGS_WITH_NAME const array.

The PhpdocAlignFixerTest::testVariadicParams() case for magic @property(...) tags also now has the PHPDoc descriptions that were missing.

Feedback is very welcome, as always.

@SpacePossum
Copy link
Contributor

SpacePossum commented Dec 14, 2021

Hi and thanks for this PR,

The tests are running, if all green than RTM from me :)

@coveralls
Copy link

coveralls commented Dec 14, 2021

Coverage Status

Coverage remained the same at 93.047% when pulling 9e13590 on antichris:phpdoc-align-property-read-and-write-with-name into 40c0419 on FriendsOfPHP:master.

@SpacePossum SpacePossum added the RTM Ready To Merge label Dec 14, 2021
@antichris
Copy link
Contributor Author

Regarding the kind/enhancement label, this PR is in fact more of a kind/bug fix.

It fixes the missing descriptions in @property(...) tags test case. Once that test case exposes that the descriptions of @property-read and @property-write really don't get aligned, this also fixes that issue.

I just spared the time and didn't open a separate issue on the bug, instead I just went ahead and fixed the thing.

@SpacePossum
Copy link
Contributor

I do like that the rule is now fixing more cases correctly, however previously it was not making incorrect changes to the code, it just didn't handle the cases. I can see both labeling; bug and enhancement, however since it didn't effect users with false negatives I prefer to tag it as enhancement. Since we'll release a new version with everything in it it doesn't matter that much in terms of release, everyone will get the fix.

@antichris
Copy link
Contributor Author

previously it was not making incorrect changes to the code, it just didn't handle the cases

Users may be getting some (unexpected) diff churn now when those cases start being handled.

it doesn't matter that much in terms of release, everyone will get the fix

Yeah, I agree. The labeling really is NBD, what really matters is that the project improves.

@SpacePossum
Copy link
Contributor

Thank you @antichris.

@SpacePossum SpacePossum merged commit cf07446 into PHP-CS-Fixer:master Dec 15, 2021
@SpacePossum SpacePossum removed the RTM Ready To Merge label Dec 15, 2021
@antichris antichris deleted the phpdoc-align-property-read-and-write-with-name branch December 15, 2021 19:42
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.

None yet

3 participants