Skip to content

feat: enable SlevomatCodingStandard.Classes.EnumCaseSpacing sniff#320

Merged
greg0ire merged 1 commit intodoctrine:13.0.xfrom
simPod:enumspacing
Oct 11, 2023
Merged

feat: enable SlevomatCodingStandard.Classes.EnumCaseSpacing sniff#320
greg0ire merged 1 commit intodoctrine:13.0.xfrom
simPod:enumspacing

Conversation

@simPod
Copy link
Copy Markdown
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 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
Copy Markdown
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
Copy Markdown
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
Copy Markdown
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
Copy Markdown
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
Copy Markdown
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
Copy Markdown
Member

greg0ire commented Oct 11, 2023

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
Copy Markdown
Member

greg0ire commented Oct 11, 2023

Thanks @simPod !

@simPod simPod deleted the enumspacing branch October 16, 2023 07:41
@simPod
Copy link
Copy Markdown
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

@greg0ire greg0ire greg0ire approved these changes

@SenseException SenseException SenseException approved these changes

@Ocramius Ocramius Awaiting requested review from Ocramius

@derrabus derrabus Awaiting requested review from derrabus

+1 more reviewer

@lcobucci lcobucci lcobucci approved these changes

Reviewers whose approvals may not affect merge requirements

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.

6 participants

@simPod @Ocramius @lcobucci @greg0ire @SenseException @derrabus