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.

Sign up for GitHub

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

Jump to bottom

feat: enable SlevomatCodingStandard.Classes.EnumCaseSpacing sniff #320

Merged
merged 1 commit into from
Oct 11, 2023
Merged

feat: enable SlevomatCodingStandard.Classes.EnumCaseSpacing sniff #320

merged 1 commit into from
Oct 11, 2023
+127 −45

Conversation

simPod
Copy link
Contributor

@simPod simPod commented Oct 9, 2023

No description provided.

@simPod simPod force-pushed the enumspacing branch 3 times, most recently from 3e5159a to 54cb016 Compare October 9, 2023 09:31
@simPod
feat: enable SlevomatCodingStandard.Classes.EnumCaseSpacing sniff

Verified

This commit was created on GitHub.com and signed with GitHub’s verified signature.
GPG key ID: B5690EEEBB952194
Verified
Learn about vigilant mode
89c3e07
@simPod simPod marked this pull request as ready for review October 9, 2023 09:36
@simPod simPod requested a review from a team as a code owner October 9, 2023 09:36
Ocramius
Ocramius previously approved these changes Oct 9, 2023
View reviewed changes
@Ocramius Ocramius added this to the 13.0.0 milestone Oct 9, 2023
@Ocramius
Copy link
Member

Ocramius commented Oct 9, 2023

This should go towards 13.0.x, which doesn't exist yet

SenseException
SenseException previously approved these changes Oct 9, 2023
View reviewed changes
Copy link
Member

@SenseException SenseException left a comment

Choose a reason for hiding this comment

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

This should go towards 13.0.x, which doesn't exist yet

I guess so. What was the reason for having a minor release like in 11.1?

@lcobucci
Copy link
Member

lcobucci commented Oct 9, 2023 •
edited
Loading

I'm unsure regarding expectations, though.

Based on the fixed file, is correct to say that we want to force enums to have exactly one, max two line breaks between cases+docblocks (and we're completely fine with a mix between them on the same enum)?

@simPod
Copy link
Contributor Author

simPod commented Oct 9, 2023

@lcobucci it is consistent with defaults in AbstractPropertyConstantAndEnumCaseSpacing so the same as we already with ConstantSpacing sniff.

It's defined in these consts:

https://github.com/slevomat/coding-standard/blob/34c47b15f852f24008c0f1534b123b2ac124f7e2/SlevomatCodingStandard/Sniffs/Classes/AbstractPropertyConstantAndEnumCaseSpacing.php#L36-L46

- max two line breaks between cases+docblocks
+ max one line break between cases+docblocks

and it's also ok to have no lines between where there are no docblocks.

@lcobucci
Copy link
Member

lcobucci commented Oct 9, 2023

I probably expressed myself too literally... One line break is kind of implicit (otherwise it'd be on the same line) but I understand the meaning 👍

Thanks!

derrabus
derrabus previously approved these changes Oct 9, 2023
View reviewed changes
@greg0ire
Copy link
Member

This should go towards 13.0.x, which doesn't exist yet

Fixed.

@lcobucci lcobucci changed the base branch from 12.0.x to 13.0.x October 11, 2023 16:39
@lcobucci lcobucci dismissed stale reviews from derrabus, SenseException, and Ocramius October 11, 2023 16:39

The base branch was changed.

@greg0ire greg0ire merged commit 0308bc4 into doctrine:13.0.x Oct 11, 2023
@greg0ire
Copy link
Member

Thanks @simPod !

@simPod simPod deleted the enumspacing branch October 16, 2023 07:41
@simPod
Copy link
Contributor Author

simPod commented Oct 16, 2023

BTW I just went through slevomat's changelog 8.11-8.14 and I know of no more possible additions to Doctrine CS.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@lcobucci lcobucci

@greg0ire greg0ire

@SenseException SenseException

@Ocramius Ocramius

@derrabus derrabus

Assignees
No one assigned
Labels
BC break Improvement New sniff
Projects
None yet
Milestone
13.0.0
Development

Successfully merging this pull request may close these issues.

None yet
6 participants
@simPod @Ocramius @lcobucci @greg0ire @SenseException @derrabus