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

DX: Add tests for type colon in backed enums #6497

Merged
merged 1 commit into from Jul 21, 2022

Conversation

jrmajor
Copy link
Contributor

@jrmajor jrmajor commented Jul 18, 2022

No description provided.

@jrmajor
Copy link
Contributor Author

jrmajor commented Jul 18, 2022

Maybe type_colon option can be dropped from SingleSpaceAfterConstructFixer in favor of ReturnTypeDeclarationFixer in the next major version?

@coveralls
Copy link

coveralls commented Jul 18, 2022

Coverage Status

Coverage remained the same at 92.898% when pulling 70d5561 on jrmajor:dx/enum-type-tests into 277c55a on FriendsOfPHP:master.

@SpacePossum
Copy link
Contributor

👍 for the new tests, nice addition :)
for the new doc line, I think the current one is more like the others we got, so +0 for me on that

@julienfalque
Copy link
Member

Maybe type_colon option can be dropped from SingleSpaceAfterConstructFixer in favor of ReturnTypeDeclarationFixer in the next major version?

I would also prefer this syntax to be fixed by return_type_declaration rather than single_space_after_construct so I'm ok with deprecating type_colon option from SingleSpaceAfterConstructFixer. However, I think support for backed enum types in ReturnTypeDeclarationFixer was not initially intended and it works by accident. This is likely why the name only mentions return types. I think we should rename the fixer.

@jrmajor
Copy link
Contributor Author

jrmajor commented Jul 19, 2022

I'm ok with deprecating type_colon option from SingleSpaceAfterConstructFixer.

I don't know how to make one of the allowed values deprecated.

However, I think support for backed enum types in ReturnTypeDeclarationFixer was not initially intended and it works by accident. This is likely why the name only mentions return types. I think we should rename the fixer.

Sure, we can't do that in a minor version though, right?

@julienfalque
Copy link
Member

I don't know how to make one of the allowed values deprecated.

You can do it like the use_trait value for option tokens of rule no_extra_blank_lines.

Sure, we can't do that in a minor version though, right?

I meant only the first step for now: copying the current fixer to a new one with the new name and deprecating the current one (with DeprecatedFixerInterface and maybe AbstractProxyFixer).

We'll actually remove the deprecated fixer when we decide to work on a major release.

Copy link
Contributor

@SpacePossum SpacePossum left a comment

Choose a reason for hiding this comment

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

I suggest merging this as-is and opening a new PR for the rename/deprecation.

@jrmajor jrmajor force-pushed the dx/enum-type-tests branch 2 times, most recently from e44bdbf to 70d5561 Compare July 21, 2022 13:02
@julienfalque julienfalque merged commit f408220 into PHP-CS-Fixer:master Jul 21, 2022
@julienfalque
Copy link
Member

Thank you @jrmajor.

@jrmajor jrmajor deleted the dx/enum-type-tests branch August 29, 2022 12:21
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

4 participants