Framework: [StyleCI] Enable no_useless_else

[carusogabriel]

• Laravel Version: 5.5.x

As suggested by @sisve in #22425, we should have a Coding Guidelines, and StyleCI has a no_useless_else fixer to help us. I couldn't find the .styleci.yml file to edit, so, how can we enable it?

[m1guelpf]

ping @GrahamCampbell

[GrahamCampbell]

👎 for adding this to the laravel preset. There are cases we don't want this.

[sisve]

So, how would the public know when to have the extra else-clauses, and when they should be removed? We can't have an endless stream of pull requests that are merged on the whim of The Merger.

[ryanwinchester]

If there are cases you don't want it, I'd imagine it's an exception to the rule. Can you not ignore specific cases in styleci?

[Stretsh]

While most devs I know believe that "useless elses" ARE the exception to the rule... Even "The Merger" seemed to have adhered to that standard...

[poppabear8883]

This is starting to annoy me tremendously ...

Where we at now ? L5.5 (soon 5.6) , PHP7 & PHP7.1 ? And we are just now discussing a style guideline for else statements ?

Are we going to be so petty to not leave this one up to the developer ?

I can't understand in this very specific case as to why this should even be a rule ? It should remain what it is .. a preference!

[GrahamCampbell]

So, how would the public know when to have the extra else-clauses, and when they should be removed? We can't have an endless stream of pull requests that are merged on the whim of The Merger.

They should just stop sending changes that add no value, frankly.

[lagbox]

Well people aren't going to stop the valueless changes now ...

When the core team isn't on the same page ... what hope is there for anyone on the outside.

[poppabear8883]

@lagbox Exactly ... well said!

[GrahamCampbell]

When the core team isn't on the same page

Nah, there's just a smaller number of people who want to change the code to not have else or elseif. If we were to change everything to that, it'd just make the code harder to read, and we'd probably get PRs from people trying to go the other way.

[poppabear8883]

Well when you have others merging valueless code randomly. What do you expect. And this is what I think @lagbox is referring to about the core team not being on the same page!

[GrahamCampbell]

I see what you mean, and don't worry, the core is on the same page. This doesn't mean contributions that change elses are always rejected.

[GrahamCampbell]

Well when you have others merging valueless code randomly.

If you say that to a new contributor, it just puts them off. It's important to find a balance, instead of just pissing everyone off.

[poppabear8883]

Yes, I do agree with that. However, its frustrating seeing PR's being closed that seems to be at least useful in some fashion, to only witness valueless changes to be merged. How does that not piss everyone off?

That is why this topic has gained so much steam. But to your point we shouldn't discourage contributions of any nature as the intent of a contribution is almost always positive. Just sometimes not necessary ;)

[Stretsh]

@GrahamCampbell said:

If you say that to a new contributor...

I've been visiting laravel/framework for the last couple of weeks due to a PR we're working on. My conclusion:

We're not talking about new contributors here. We're talking about people who submit these kinds of minimal PRs one after the other on numerous packages, just like folks on Stackoverflow, who just post to rack up their number of contributions.

From experience I can tell you that REAL new contributors can be put off by this behavior. In my case the idea that personal preferences find their way into standards of a framework of this size is kinda off putting, especially when it's very badly documented. The latter makes it even worse, because Taylor can then arbitrarily merge or reject, without adhering to standards.

As part of the organization I want you to know that I have been seriously considering switching frameworks in this last week. Yes, I'm just 1 in a few million, but I believe that, if you don't like the culture within a group/organization, and you can't do anything to change it, move along.

(This turned into something longer that I intended, but wanted to communicate my experience as "new contributor")

[carusogabriel]

We're talking about people who submit these kinds of minimal PRs one after the other on numerous packages, just like folks on Stackoverflow, who just post to rack up their number of contributions.

In my defense, I’ve been contributing to Laravel since July. Yes, is minimal PRs, but is what I can do for now, since I’m not experient in PHP. Sorry about the inconvenience, and please @grahamcampbell, if you can lock this conversation, this is starting to annoying me.

[poppabear8883]

since I’m not an expertise in PHP

[Stretsh]

@carusogabriel said:

In my defense, I’ve been contributing to Laravel since July. Yes, is minimal PRs, but is what I can do for now, since I’m not experient in PHP.

No, my friend, this one is not on you! It might be your contribution that triggered this discussion, but the center of the frustration is more in the way stuff happens.

The lack of documentation for preferred style here makes it difficult for new contributors to know what to adhere to. It then sucks when your PR, which took you weeks to research and setup, is just closed because of something simple, while a lot of minimal PRs are accepted.

You submit a PR to remove elses. You can and should! It is up to the maintainers to decide whether or not to accept it and to decide that their project should not force all users to adhere to the "best practices" used by a group.

And THAT is where the frustration lies. Imagine working on a fix for weeks and then while waiting for it to be merged, someone else submits a frivolous change, which affects yours massively. And then the maintainer decides to switch positions and accept that preferential change after denying them for ages. This, before your work is accepted.

On top of that, add to it, that this would be your first contribution (hence, you are a new contributor).

So I feel for you, that this happens on your count. But some stuff just needed to put out there.

[GrahamCampbell]

Just to be clear. There is no standard as to if we want trailing else clauses, because it depends on readability. One thing we could documentation is to never send code style pull requests. We already automate our code style. I'm pretty sure we do document that, but I guess we could be more explicit.

[cbj4074]

I see what you mean, and don't worry, the core is on the same page. This doesn't mean contributions that change elses are always rejected.

It doesn't? Can anyone point to another PR that changes only elses and was merged (besides the one that lead to this discussion)?

Maybe it's just me, but I still feel that we are owed an explanation as to why this PR was merged when the precedent, historically, has been not to bother.

@GrahamCampbell So, which is it? While I agree with the overall premise of what you're saying, you are leaving room for ambiguity with the above-quoted statement, which conflicts with other statements you've made in this thread.

We already automate our code style. I'm pretty sure we do document that, but I guess we could be more explicit.

You do:

Don't worry if your code styling isn't perfect! StyleCI will automatically merge any style fixes into the Laravel repository after pull requests are merged. This allows us to focus on the content of the contribution and not the code style. [emphasis mine]

Yes, I would say it needs to be more explicit.

and please @GrahamCampbell, if you can lock this conversation, this is starting to annoying me.

That is truly rich. Priceless, even.

I would submit the PR myself to clarify the language if I wasn't convinced it would be rejected.

[browner12]

I think we should paint the bike shed red.