Skip to content

Add new "Endless Methods" rule#858

Merged
bbatsov merged 1 commit intorubocop:masterfrom
dvandersluis:endless-methods
Jan 11, 2021
Merged

Add new "Endless Methods" rule#858
bbatsov merged 1 commit intorubocop:masterfrom
dvandersluis:endless-methods

Conversation

@dvandersluis
Copy link
Copy Markdown
Member

@dvandersluis dvandersluis commented Dec 22, 2020

Endless methods are about to be added by ruby 3.0 but IMO they are just another form of single-line methods (which we have a rule for) and make it harder to read the structure of a ruby file.

After discussion it was decided to have the style guide allow endless methods, but only those that are a single line.

@marcandre
Copy link
Copy Markdown
Contributor

marcandre commented Dec 22, 2020

That's my first impression too and I voiced my concerns at the time. Since this was announced I've been realizing that there are indeed many single line methods. I might experiment in my personal style for simple methods, say with at most one argument.

In short, maybe we should give it time and see how good or bad it can be?

@bbatsov
Copy link
Copy Markdown
Collaborator

bbatsov commented Dec 23, 2020

I'm in favour of the guideline, even though I assume some people like the new syntax. I'm in no rush to merge this, though.

@koic
Copy link
Copy Markdown
Member

koic commented Dec 23, 2020

I also look forward to feedback from community :-)

@bbatsov
Copy link
Copy Markdown
Collaborator

bbatsov commented Dec 23, 2020

I've dropped a tweet on topic https://twitter.com/bbatsov/status/1341653482592464896 and referred to this discussion. I guess we'll get some feedback soon.

@coding-red-panda
Copy link
Copy Markdown

coding-red-panda commented Dec 23, 2020

I vote to not recommend them. As said they are just another form of one liners and we already have good cops for them

@bbatsov
Copy link
Copy Markdown
Collaborator

bbatsov commented Dec 23, 2020

Btw, when it comes to rationale - my biggest gripe with them is that they introduce (yet another inconsistency) in Ruby's syntax. def forms were always delimited with an end and this was easy to work with. A lot of editors would automatically insert an end the moment you type def, now they we'll have to do extra work to figure out which kind of def you meant to use.

@atipugin
Copy link
Copy Markdown

Btw, when it comes to rationale - my biggest gripe with them is that they introduce (yet another inconsistency) in Ruby's syntax. def forms were always delimited with an end and this was easy to work with. A lot of editors would automatically insert an end the moment you type def, now they we'll have to do extra work to figure out which kind of def you meant to use.

Fully agree here. Endless methods add nothing but inconsistency.

@bbatsov
Copy link
Copy Markdown
Collaborator

bbatsov commented Dec 23, 2020

And a couple of syntax edge cases that annoy me:

# syntax error
def foo = bar

# correct
def foo() = bar

# syntax error
def foo() = bar baz

# correct
def foo() = bar(baz)

The thing I dislike most in any language is having "special cases" and this syntax is a case study in adding special cases. :-)

@coding-red-panda
Copy link
Copy Markdown

And a couple of syntax edge cases that annoy me:

# syntax error
def foo = bar

# correct
def foo() = bar

# syntax error
def foo() = bar baz

# correct
def foo() = bar(baz)

The thing I dislike most in any language is having "special cases" and this syntax is a case study in adding special cases. :-)

This smells like JavaScript for me.....not Ruby

@mame
Copy link
Copy Markdown

mame commented Dec 23, 2020

def foo = bar is valid in the current Ruby master.

IMO, the syntax should be used only when it is in one line and the body has a simple expression without side effect.

# OK
def the_answer = 42
def get_x = @x
def square(x) = x * x

# Not good: has multiple lines
def fib(x) = if x < 2
  x
else
  fib(x - 1) + fib(x - 2)
end

# Not (so) good: has side effect (I have no idea if Rubocop can check this or not)
def set_x(x) = (@x = x)
def print_foo = puts("foo")

@bbatsov
Copy link
Copy Markdown
Collaborator

bbatsov commented Dec 23, 2020

IMO, the syntax should be used only when it is in one line and the body has a simple expression without side effect.

@mame Thanks for the great feedback! I guess we can rework the guideline to be something in line with your suggestion, as it certainly makes sense to me.

@coding-red-panda
Copy link
Copy Markdown

I have one question then, more out of curiosity.
If you say that the following is "OK"

# OK
def the_answer = 42
def get_x = @x
def square(x) = x * x

Then

  • Why not use a constant for the first?
  • This violates the Style/Getters Cop, can't use get_, and then the x method can be replaced with attr_reader
  • This one I'd be okay with, actually.

@mame
Copy link
Copy Markdown

mame commented Dec 23, 2020

Why not use a constant for the first?

I think that sometimes a class design requires it to be a method. The most trivial case is, maybe, a callback or event hook.

class SomeEventHandler
  def on_interested_event_hook
    # process the event...
  end

  def on_uninterested_event_hook = nil
end

This violates the Style/Getters Cop, can't use get_, and then the x method can be replaced with attr_reader

Yes, attr_reader is preferable if it can be used. But sometimes I want to use other method name than a instance variable:

def foo = @real_foo

@coding-red-panda
Copy link
Copy Markdown

Thanks. Great example with the callbacks, that's definitely a valid use-case in Rails that could warrant the usage of these methods to keep the controllers thin and support the required behavior!

I get the second argument, but I personally feel it's not strong enough to convince me of the usage for that use-case.
But glad to had the discussion. I'm personally still not in favor of their usage, but community decides :)

@marcandre
Copy link
Copy Markdown
Contributor

And a couple of syntax edge cases that annoy me

FYI there's an issue opened to address that: https://bugs.ruby-lang.org/issues/17398

@Drenmi
Copy link
Copy Markdown
Contributor

Drenmi commented Dec 24, 2020

Hell is optional syntax.
-- Sartre

Jokes aside, I'd love to have a configurable cop to ban or force consistent usage. 🙂

I would like to not make it seem as if RuboCop is "taking a stance" on which one. There is enough energy spent on issues and PRs opened to shoehorn someone's personal preference as the default because they mistake the default for The Way™. 😅

@dvandersluis
Copy link
Copy Markdown
Member Author

dvandersluis commented Jan 11, 2021

FYI a new Style/EndlessMethod cop was added in RuboCop 1.8.0. It defaults to allowing endless methods, if they are on a single line (but is configurable).

@bbatsov
Copy link
Copy Markdown
Collaborator

bbatsov commented Jan 11, 2021

@dvandersluis We should update this PR to reflect this.

@dvandersluis
Copy link
Copy Markdown
Member Author

@bbatsov agreed! I've updated the wording now.

@dvandersluis dvandersluis changed the title Add new "No Endless Methods" rule Add new "Endless Methods" rule Jan 11, 2021
@bbatsov
Copy link
Copy Markdown
Collaborator

bbatsov commented Jan 11, 2021

@dvandersluis I think it'd be better if you used the examples by @mame.

@dvandersluis
Copy link
Copy Markdown
Member Author

dvandersluis commented Jan 11, 2021

I've copied them verbatim now so let me know if you want me to change anything 😁

@bbatsov bbatsov merged commit 35a5310 into rubocop:master Jan 11, 2021
@bbatsov
Copy link
Copy Markdown
Collaborator

bbatsov commented Jan 11, 2021

I think we're good for now. Thanks!

@marcandre
Copy link
Copy Markdown
Contributor

marcandre commented Jan 11, 2021

Why is the new rule stating "If single-line methods are to be used, prefer endless methods"?

The discussion went from disallowing endless methods to favoring them??

Edit: I image that single-line methods mean fitting in one line, not with a single line body...

@dvandersluis
Copy link
Copy Markdown
Member Author

@marcandre yeah, it follows the section on single-line methods (which is the previous section of the style guide). It's meant to mean methods on a single line, not with a single line body (and maybe that needs to be clarified).

FWIW I'm still on team no-endless methods 😆

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.

8 participants