Skip to content

Remove switch case - #7729

Merged
mrmckeb merged 2 commits into
react:masterfrom
andrelmlins:master
Oct 1, 2019
Merged

Remove switch case#7729
mrmckeb merged 2 commits into
react:masterfrom
andrelmlins:master

Conversation

@andrelmlins

Copy link
Copy Markdown
Contributor

In the file react-scripts replaces the switch case with a simple if else. Since there was only one case, it would be nice to replace it with an if else.

@facebook-github-bot

Copy link
Copy Markdown

Thank you for your pull request and welcome to our community. We require contributors to sign our Contributor License Agreement, and we don't seem to have you on file. In order for us to review and merge your code, please sign up at https://code.facebook.com/cla. If you are contributing on behalf of someone else (eg your employer), the individual CLA may not be sufficient and your employer may need the corporate CLA signed.

If you have received this in error or have any questions, please contact us at cla@fb.com. Thanks!

@facebook-github-bot

Copy link
Copy Markdown

Thank you for signing our Contributor License Agreement. We can now accept your code for this (and any) Facebook open source project. Thanks!

@mrmckeb

Copy link
Copy Markdown
Contributor

Hi @andrelmlins, thanks for this clean-up. I don't know if there was a reason this was written like this originally, maybe there's a plan to extend the behaviour.

@ianschmitz or @iansu - any ideas?

@iansu

Copy link
Copy Markdown
Contributor

I'm conflicted on this one because I strongly dislike switch statements but I think it was written that way to make it clear exactly which commands that block of code applies to. Something like this might be best: if (['build', 'eject', 'start', 'test'].includes(script)

@andrelmlins

Copy link
Copy Markdown
ContributorAuthor

Do I make this change?

@iansu

Copy link
Copy Markdown
Contributor

@andrelmlins Yes, please make this change.

@andrelmlins

Copy link
Copy Markdown
ContributorAuthor

Done

@mrmckeb

Copy link
Copy Markdown
Contributor

Restarting the CI.

@mrmckebmrmckeb closed this Oct 1, 2019
@mrmckebmrmckeb reopened this Oct 1, 2019
@mrmckeb

Copy link
Copy Markdown
Contributor

Looks great, and I see you've made @iansu's suggested change. Thanks for the contribution!

@mrmckeb
mrmckeb merged commit f36d61a into react:masterOct 1, 2019
@locklockBot locked and limited conversation to collaborators Oct 6, 2019
Sign up for freeto subscribe to this conversation on GitHub. Already have an account? Sign in.

Projects

None yet

Development

Successfully merging this pull request may close these issues.

4 participants

@andrelmlins@facebook-github-bot@mrmckeb@iansu