Rubocop: turning off the Style/RedundantReturn

Hi,

I have a PR regarding orchestration [1] (orchestration methods returning false accidentally
even though there was no error for funny reasons such as changing logging levels or
puppet ca proxy not being assigned to a puppet ca). I'm proposing turning off the ReduntantReturn style
as there are places (such as here), where the return can emphasize that the return
value is really significant, and the standalone boolean at the end of methods just doesn't look good.

I'm suggesting turning this off globally, as I believe (and maybe I'm alone here) that there
are situations where the explicit return can be justified (to emphasize the return value is significant
where it might look like it isn't) and I don't want for people to feel guilty about that :slight_smile:

Thoughts?

[1] - https://github.com/theforeman/foreman/pull/3269

– Ivan

You're not alone. I prefer to be able to call return explicitly. I'm missing
return in following example

def answer(q)
if q == 'question'
return 'answer'
end
42
end

I don't see any negatives. Based on PR discussion, I don't understand why it
would require extra effort from reviewer. It's fine when someone uses it, just
ignore it.

So +1 from me for disabling globally.

··· On Tuesday 29 of March 2016 10:00:00 Ivan Necas wrote: > Hi, > > I have a PR regarding orchestration [1] (orchestration methods returning > false accidentally even though there was no error for funny reasons such as > changing logging levels or puppet ca proxy not being assigned to a puppet > ca). I'm proposing turning off the ReduntantReturn style as there are > places (such as here), where the return can emphasize that the return value > is really significant, and the standalone boolean at the end of methods > just doesn't look good. > > I'm suggesting turning this off globally, as I believe (and maybe I'm alone > here) that there are situations where the explicit return can be justified > (to emphasize the return value is significant where it might look like it > isn't) and I don't want for people to feel guilty about that :) > > Thoughts?


Marek

> > Hi,
> >
> > I have a PR regarding orchestration [1] (orchestration methods returning
> > false accidentally even though there was no error for funny reasons such as
> > changing logging levels or puppet ca proxy not being assigned to a puppet
> > ca). I'm proposing turning off the ReduntantReturn style as there are
> > places (such as here), where the return can emphasize that the return value
> > is really significant, and the standalone boolean at the end of methods
> > just doesn't look good.

Doesn't look good is very subjective. It does look good to me? Calling
return in ruby other than in a guard clause (shortcut exit to a method)
isn't standard

> >
> > I'm suggesting turning this off globally, as I believe (and maybe I'm alone
> > here) that there are situations where the explicit return can be justified
> > (to emphasize the return value is significant where it might look like it
> > isn't) and I don't want for people to feel guilty about that :slight_smile:
> >
> > Thoughts?
>
> You're not alone. I prefer to be able to call return explicitly. I'm missing
> return in following example
>
> def answer(q)
> if q == 'question'
> return 'answer'
> end
> 42
> end

I tend to abide by the ruby style guide - that method would be OK (and
idiomatic) like

def answer(q)
return 'answer' if q == 'question'
42
end

Adding the option of having return wherever just makes the code base
less coherent. Especially if it has hidden meanings like 'hey this
return means something'.

··· On 03/30, Marek Hulán wrote: > On Tuesday 29 of March 2016 10:00:00 Ivan Necas wrote:

All in all it’s mostly a style convention, which is why Rubocop exists
and 1000s of rubyists decide to use it, to stop these discussions from
happening and standardize on one style. All major style guides use it
(fallacy of appeal to authority, I know), for a reason - so that code
looks like it was written by one person and it’s more readable.

The whole point for that cop to exist is to stop people from discussing
about this, and spend that time writing (coherent, homogeneous) code.

I don’t see any negatives. Based on PR discussion, I don’t understand why it
would require extra effort from reviewer. It’s fine when someone uses it, just
ignore it.

So +1 from me for disabling globally.


Marek


You received this message because you are subscribed to the Google Groups “foreman-dev” group.
To unsubscribe from this group and stop receiving emails from it, send an email to foreman-dev+unsubscribe@googlegroups.com.
For more options, visit https://groups.google.com/d/optout.


Daniel Lobato Garcia

@dLobatog

GPG: http://keys.gnupg.net/pks/lookup?op=get&search=0x7A92D6DD38D6DE30
Keybase: elobato (Daniel Lobato Garcia) | Keybase

I tend to agree with Daniel. Personally I find it confusing to see a method
that ends in a return statement as I’ve come to associate return with a
change in the flow of control. I also worry that if we disable the cop,
we’re going to see a mixture of methods that either do or do not have
returns.

If someone really wants to use the return keyword to clarify something, I
am fine with just disabling the cop on a single line or file. But +1 to
keeping the cop on globally from me.

David

··· On Wed, Mar 30, 2016 at 6:37 AM, Daniel Lobato Garcia wrote:

On 03/30, Marek Hulán wrote:

On Tuesday 29 of March 2016 10:00:00 Ivan Necas wrote:

Hi,

I have a PR regarding orchestration [1] (orchestration methods
returning

false accidentally even though there was no error for funny reasons
such as

changing logging levels or puppet ca proxy not being assigned to a
puppet

ca). I’m proposing turning off the ReduntantReturn style as there are
places (such as here), where the return can emphasize that the return
value

is really significant, and the standalone boolean at the end of methods
just doesn’t look good.

Doesn’t look good is very subjective. It does look good to me? Calling
return in ruby other than in a guard clause (shortcut exit to a method)
isn’t standard

I’m suggesting turning this off globally, as I believe (and maybe I’m
alone

here) that there are situations where the explicit return can be
justified

(to emphasize the return value is significant where it might look like
it

isn’t) and I don’t want for people to feel guilty about that :slight_smile:

Thoughts?

You’re not alone. I prefer to be able to call return explicitly. I’m
missing
return in following example

def answer(q)
if q == 'question’
return 'answer’
end
42
end

I tend to abide by the ruby style guide - that method would be OK (and
idiomatic) like

def answer(q)
return ‘answer’ if q == 'question’
42
end

Adding the option of having return wherever just makes the code base
less coherent. Especially if it has hidden meanings like ‘hey this
return means something’.


All in all it’s mostly a style convention, which is why Rubocop exists
and 1000s of rubyists decide to use it, to stop these discussions from
happening and standardize on one style. All major style guides use it
(fallacy of appeal to authority, I know), for a reason - so that code
looks like it was written by one person and it’s more readable.

https://github.com/bbatsov/ruby-style-guide
https://github.com/airbnb/ruby
https://github.com/styleguide/ruby

The whole point for that cop to exist is to stop people from discussing
about this, and spend that time writing (coherent, homogeneous) code.

I don’t see any negatives. Based on PR discussion, I don’t understand
why it
would require extra effort from reviewer. It’s fine when someone uses
it, just
ignore it.

So +1 from me for disabling globally.


Marek


You received this message because you are subscribed to the Google
Groups “foreman-dev” group.
To unsubscribe from this group and stop receiving emails from it, send
an email to foreman-dev+unsubscribe@googlegroups.com.
For more options, visit https://groups.google.com/d/optout.


Daniel Lobato Garcia

@dLobatog
blog.daniellobato.me
daniellobato.me

GPG: http://keys.gnupg.net/pks/lookup?op=get&search=0x7A92D6DD38D6DE30
Keybase: https://keybase.io/elobato


You received this message because you are subscribed to the Google Groups
"foreman-dev" group.
To unsubscribe from this group and stop receiving emails from it, send an
email to foreman-dev+unsubscribe@googlegroups.com.
For more options, visit https://groups.google.com/d/optout.

Obviously that was artificial example and the whole method could be one
expression with ternary operator but that wasn't the point. If the logic above
42 was more complex, the 42 itself seems easier to miss. And (subjectively)
doesn't look good :slight_smile:

Anyway I'd be happy to hear more (subjective) opinions, then choose the most
favorite style and use it.

··· On Wednesday 30 of March 2016 12:37:16 Daniel Lobato Garcia wrote: > On 03/30, Marek Hulán wrote: > > On Tuesday 29 of March 2016 10:00:00 Ivan Necas wrote: > > > Hi, > > > > > > I have a PR regarding orchestration [1] (orchestration methods returning > > > false accidentally even though there was no error for funny reasons such > > > as > > > changing logging levels or puppet ca proxy not being assigned to a > > > puppet > > > ca). I'm proposing turning off the ReduntantReturn style as there are > > > places (such as here), where the return can emphasize that the return > > > value > > > is really significant, and the standalone boolean at the end of methods > > > just doesn't look good. > > Doesn't look good is very subjective. It does look good to me? Calling > `return` in ruby other than in a guard clause (shortcut exit to a method) > isn't standard > > > > I'm suggesting turning this off globally, as I believe (and maybe I'm > > > alone > > > here) that there are situations where the explicit return can be > > > justified > > > (to emphasize the return value is significant where it might look like > > > it > > > isn't) and I don't want for people to feel guilty about that :) > > > > > > Thoughts? > > > > You're not alone. I prefer to be able to call return explicitly. I'm > > missing return in following example > > > > def answer(q) > > > > if q == 'question' > > > > return 'answer' > > > > end > > 42 > > > > end > > I tend to abide by the ruby style guide - that method would be OK (and > idiomatic) like > > def answer(q) > return 'answer' if q == 'question' > 42 > end


Marek

Adding the option of having return wherever just makes the code base
less coherent. Especially if it has hidden meanings like ‘hey this
return means something’.


All in all it’s mostly a style convention, which is why Rubocop exists
and 1000s of rubyists decide to use it, to stop these discussions from
happening and standardize on one style. All major style guides use it
(fallacy of appeal to authority, I know), for a reason - so that code
looks like it was written by one person and it’s more readable.

https://github.com/bbatsov/ruby-style-guide
https://github.com/airbnb/ruby
https://github.com/styleguide/ruby

The whole point for that cop to exist is to stop people from discussing
about this, and spend that time writing (coherent, homogeneous) code.

I don’t see any negatives. Based on PR discussion, I don’t understand why
it would require extra effort from reviewer. It’s fine when someone uses
it, just ignore it.

So +1 from me for disabling globally.


Marek


You received this message because you are subscribed to the Google Groups
"foreman-dev" group. To unsubscribe from this group and stop receiving
emails from it, send an email to
foreman-dev+unsubscribe@googlegroups.com. For more options, visit
https://groups.google.com/d/optout.


Daniel Lobato Garcia

@dLobatog
blog.daniellobato.me
daniellobato.me

GPG: http://keys.gnupg.net/pks/lookup?op=get&search=0x7A92D6DD38D6DE30
Keybase: https://keybase.io/elobato

Thanks for the opinions so far.

I agree that it would be nicer, for the procedure-like method (as we do have in orchestration), to not expect the
return value, but rather have other mechanism to indicate a failure but I'm not sure we can change that without breaking some backward compatibilities.

I share the Marek's taste for the case he describes (here is a real-world example https://github.com/theforeman/foreman/blob/1.11.0/app/models/concerns/orchestration/dns.rb#L173) - it just doesn't look good to me and explicit return would make it clear what the method returns (and not just accidentally returning the last statement that happened to be the required value).

It seems we have 3:2 in favor of keeping the cop on. I leaving this thread opened until Monday (including). If no other folks express their preference, I will take it as they are happy about how it's done right now, which is something I'm not very happy about, but we can't make everyone happy, when it comes to enabling/disabling cops. I still think the discussion about enabled cops in general is important, as I don't think all the cops make the world a better place (as in real-world, there are good and bad cops:).

– Ivan

··· ----- Original Message ----- > On Wednesday 30 of March 2016 12:37:16 Daniel Lobato Garcia wrote: > > On 03/30, Marek Hulán wrote: > > > On Tuesday 29 of March 2016 10:00:00 Ivan Necas wrote: > > > > Hi, > > > > > > > > I have a PR regarding orchestration [1] (orchestration methods > > > > returning > > > > false accidentally even though there was no error for funny reasons > > > > such > > > > as > > > > changing logging levels or puppet ca proxy not being assigned to a > > > > puppet > > > > ca). I'm proposing turning off the ReduntantReturn style as there are > > > > places (such as here), where the return can emphasize that the return > > > > value > > > > is really significant, and the standalone boolean at the end of methods > > > > just doesn't look good. > > > > Doesn't look good is very subjective. It does look good to me? Calling > > `return` in ruby other than in a guard clause (shortcut exit to a method) > > isn't standard > > > > > > I'm suggesting turning this off globally, as I believe (and maybe I'm > > > > alone > > > > here) that there are situations where the explicit return can be > > > > justified > > > > (to emphasize the return value is significant where it might look like > > > > it > > > > isn't) and I don't want for people to feel guilty about that :) > > > > > > > > Thoughts? > > > > > > You're not alone. I prefer to be able to call return explicitly. I'm > > > missing return in following example > > > > > > def answer(q) > > > > > > if q == 'question' > > > > > > return 'answer' > > > > > > end > > > 42 > > > > > > end > > > > I tend to abide by the ruby style guide - that method would be OK (and > > idiomatic) like > > > > def answer(q) > > return 'answer' if q == 'question' > > 42 > > end > > Obviously that was artificial example and the whole method could be one > expression with ternary operator but that wasn't the point. If the logic > above > 42 was more complex, the 42 itself seems easier to miss. And (subjectively) > doesn't look good :-) > > Anyway I'd be happy to hear more (subjective) opinions, then choose the most > favorite style and use it. > > -- > Marek > > > > > Adding the option of having `return` wherever just makes the code base > > less coherent. Especially if it has hidden meanings like 'hey this > > return means something'. > > > > ---------------- > > > > All in all it's mostly a style convention, which is why Rubocop exists > > and 1000s of rubyists decide to use it, to stop these discussions from > > happening and standardize on one style. All major style guides use it > > (fallacy of appeal to authority, I know), for a reason - so that code > > looks like it was written by one person and it's more readable. > > > > https://github.com/bbatsov/ruby-style-guide > > https://github.com/airbnb/ruby > > https://github.com/styleguide/ruby > > > > The whole point for that cop to exist is to stop people from discussing > > about this, and spend that time writing (coherent, homogeneous) code. > > > > > I don't see any negatives. Based on PR discussion, I don't understand why > > > it would require extra effort from reviewer. It's fine when someone uses > > > it, just ignore it. > > > > > > So +1 from me for disabling globally. > > > > > > -- > > > Marek > > > > > > -- > > > You received this message because you are subscribed to the Google Groups > > > "foreman-dev" group. To unsubscribe from this group and stop receiving > > > emails from it, send an email to > > > foreman-dev+unsubscribe@googlegroups.com. For more options, visit > > > https://groups.google.com/d/optout. > > > > -- > > Daniel Lobato Garcia > > > > @dLobatog > > blog.daniellobato.me > > daniellobato.me > > > > GPG: http://keys.gnupg.net/pks/lookup?op=get&search=0x7A92D6DD38D6DE30 > > Keybase: https://keybase.io/elobato > > -- > You received this message because you are subscribed to the Google Groups > "foreman-dev" group. > To unsubscribe from this group and stop receiving emails from it, send an > email to foreman-dev+unsubscribe@googlegroups.com. > For more options, visit https://groups.google.com/d/optout. >