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

Ensure that spacing around cases are consistent #169

Merged
merged 1 commit into from
Apr 6, 2020

Conversation

carusogabriel
Copy link
Contributor

As reported via slevomat/coding-standard#867, in case that a switch
with cases returning or breaking, the JumpStatementsSpacing sniff
wasn't handling it properly.

These 3 new options ensure that this sniff handles this case as well.

Requested by @morozov via #167 (comment).

@carusogabriel carusogabriel added this to the 8.0.0 milestone Apr 4, 2020
@carusogabriel carusogabriel requested a review from a team as a code owner April 4, 2020 13:27
@ostrolucky
Copy link
Member

Isn't there better alternative than having same input and fixed file?

morozov
morozov previously approved these changes Apr 4, 2020
Copy link
Member

@morozov morozov left a comment

Choose a reason for hiding this comment

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

I'm approving the addition of the configuration as such (thanks, @carusogabriel!) but the #169 (comment) concern needs to be taken into account.

The original issue was that the properly formatted code was reported as containing errors. I don't know if the sniff is supposed to fix it or just not report.

@carusogabriel
Copy link
Contributor Author

I don't know if the sniff is supposed to fix it or just not report.

The sniff is just not reporting it, that's why both input and fixed codes are equal.

If you remove the added options in the Ruleset file, it'll break.

@ostrolucky ostrolucky force-pushed the improvement/spacing-case-return branch 2 times, most recently from ca32067 to 05ba49c Compare April 4, 2020 17:29
ostrolucky
ostrolucky previously approved these changes Apr 4, 2020
Copy link
Member

@ostrolucky ostrolucky left a comment

Choose a reason for hiding this comment

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

Here, this should be better I think. Those options actually affect result of the diff so I think it's useful to show what they change

greg0ire
greg0ire previously approved these changes Apr 4, 2020
As reported via slevomat/coding-standard#867, in case that a switch
with cases returning or breaking, the `JumpStatementsSpacing` sniff
wasn't handling it properly.

These 3 new options added ensure that this sniff handles this case as well.
@ostrolucky ostrolucky dismissed stale reviews from greg0ire and themself via 1f1aa19 April 4, 2020 18:30
@ostrolucky ostrolucky force-pushed the improvement/spacing-case-return branch from 05ba49c to 1f1aa19 Compare April 4, 2020 18:31
@ostrolucky ostrolucky requested a review from greg0ire April 6, 2020 08:16
@alcaeus alcaeus merged commit d45826c into master Apr 6, 2020
@alcaeus alcaeus deleted the improvement/spacing-case-return branch April 6, 2020 10:50
@alcaeus
Copy link
Member

alcaeus commented Apr 6, 2020

Thanks @carusogabriel and @morozov for the suggestion!

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

6 participants