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

[5.2] Remove unnecessary else #14036

Merged
merged 1 commit into from Jun 18, 2016
Merged

Conversation

greydnls
Copy link
Contributor

This PR:

  • Removes an unnecessary else. Since you're returning in the if block above, you don't need an else statement.

@lucasmichot
Copy link
Contributor

Many PR like this have been refused before.

@greydnls
Copy link
Contributor Author

Why?

@lucasmichot
Copy link
Contributor

#8682 (comment)

@tw2113
Copy link

tw2113 commented Jun 18, 2016

a referral to a comment that says the same thing, doesn't validate the refusal. I can understand not wanting to accept because of $reason, but at least provide a link to a valid reason.

@greydnls greydnls changed the title Remove unnecessary else [5.2] Remove unnecessary else Jun 18, 2016
@lucasmichot
Copy link
Contributor

Graham will give you one

Le samedi 18 juin 2016, Michael Beckwith notifications@github.com a
écrit :

a referral to a comment that says the same thing, doesn't validate the
refusal. I can understand not wanting to accept because of $reason, but at
least provide a link to a valid reason.


You are receiving this because you commented.
Reply to this email directly, view it on GitHub
#14036 (comment),
or mute the thread
https://github.com/notifications/unsubscribe/AAfWQ7PJVztTUAXYHKiUAlepEGruQn7gks5qM5pHgaJpZM4I44z5
.

@GrahamCampbell
Copy link
Member

Both this change, and the reverse make valid pull requests. Thanks, but we won't be merging PRs like this.

@greydnls
Copy link
Contributor Author

What the reasoning behind that? It's a tiny change that breaks no logic and improves the code readability. Things like this are the easiest way for new developers to start contributing to OSS. I'm interested in why one of the largest frameworks in the ecosystem would deny requests like this with no further explanation.

@midorikocak
Copy link

you can't fight egoist bastards. start your own project.

@assertchris
Copy link
Contributor

@midorikocak this could simply be a failure to communicate. I don't think insults are called for or even helpful at this point.

@assertchris
Copy link
Contributor

@GrahamCampbell is it fair to say that only those with merge rights to this repo are allowed to contribute subjective/style-based changes. Everything else needs to be a functional change?

@laravel laravel locked and limited conversation to collaborators Jun 18, 2016
@GrahamCampbell
Copy link
Member

Thank you, but as I said, we're not looking to merge changes like this. I wrote StyleCI to automate our code style so we don't ever have to waste time processing code style issues again.

@taylorotwell
Copy link
Member

taylorotwell commented Jun 18, 2016

Graham isn't even supposed to be closing or merging PRs in the first place as I've told him multiple times so not sure why this is closed. That's something I can discuss with him privately. I am the only one who is supposed to either close or merge a PR.

Thanks for the PR.

@taylorotwell taylorotwell reopened this Jun 18, 2016
@laravel laravel unlocked this conversation Jun 18, 2016
@taylorotwell taylorotwell merged commit 0934096 into laravel:5.2 Jun 18, 2016
@greydnls
Copy link
Contributor Author

Thanks, @taylorotwell!

@GrahamCampbell
Copy link
Member

Really sorry everyone. I really didn't mean to upset anyone here.

@ibrasho
Copy link
Contributor

ibrasho commented Jun 18, 2016

Sometimes being welcoming to contributions is more important than sticking to rules and guidelines.

@GrahamCampbell
Copy link
Member

Totally agree. Really sorry I processed this in this way. In future, I will not be closing any PRs at all.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

8 participants