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

Add a test case with switch cases containing return #145

Closed
wants to merge 1 commit into from
Closed

Add a test case with switch cases containing return #145

wants to merge 1 commit into from

Conversation

Lctrs
Copy link
Contributor

@Lctrs Lctrs commented Dec 9, 2019

No description provided.

@Lctrs
Copy link
Contributor Author

Lctrs commented Dec 9, 2019

Seeing this in one of my project when trying to upgrade to 7.0.0. phpcbf failed to autofix one of my file which contains a return inside a switch case.

Maybe 2 sniffers that collide ?

@lcobucci
Copy link
Member

lcobucci commented Dec 9, 2019

@Lctrs thanks for you PR!

Would you please move your test to another file? We're trying to isolate things as much as possible and only modifying existing ones when adding a sniff that requires change there.

Maybe 2 sniffers that collide ?

That does seem to be the case.

@Lctrs
Copy link
Contributor Author

Lctrs commented Dec 9, 2019

@lcobucci Done.

I tried to add differents testing cases :

  • break followed by case
  • return followed by case
  • case with a body and a return followed by case
  • return followed by default

@Ocramius Ocramius added this to the 7.0.1 milestone Dec 9, 2019
@Ocramius Ocramius added the bug label Dec 9, 2019
@lcobucci
Copy link
Member

lcobucci commented Dec 9, 2019

@Lctrs that's awesome, thanks for! I'll take a look at it ASAP.

@lcobucci lcobucci self-requested a review December 9, 2019 21:43
@Lctrs
Copy link
Contributor Author

Lctrs commented Dec 9, 2019

I just tested locally with #146 and it fixed the problem for me. 🎉

@Ocramius
Copy link
Member

Ocramius commented Dec 9, 2019

Hmm, we can't get this through unless green though: is the failure related to this patch?

@Lctrs
Copy link
Contributor Author

Lctrs commented Dec 9, 2019

@Ocramius I can incorporate this test into #146.

@Ocramius
Copy link
Member

Moving to 7.0.2 (if still relevant)

@Ocramius Ocramius modified the milestones: 7.0.1, 7.0.2 Dec 11, 2019
@Lctrs Lctrs changed the title Add failing test for failed to auto fix when a switch case contains a return Add a test case with switch cases containing return Dec 11, 2019
@Lctrs
Copy link
Contributor Author

Lctrs commented Dec 11, 2019

@Ocramius It works now thanks to #146. I updated the PR to add a test case in case you have any interest in it. Otherwise, you can close it 👍

@Ocramius Ocramius self-assigned this Dec 11, 2019
@Ocramius
Copy link
Member

Ocramius commented Dec 11, 2019

Thanks for the ping! (closing: I think the process is not needed for now)

@Ocramius Ocramius closed this Dec 11, 2019
@Lctrs Lctrs deleted the return-in-switch-case branch December 11, 2019 10: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