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

Lint/UselessMethodDefinition marks empty initialize with arguments as a violation #8626

Closed
andycandrea opened this issue Sep 1, 2020 · 8 comments
Labels

Comments

@andycandrea
Copy link

Hi Rubocop team,

I just upgraded to Rubocop 0.90.0 and enabled the Lint/UselessMethodDefinition cop. After running it on my application, I came across a case where I find this cop's current behavior a little misleading: it marks empty initialize methods that accept arguments as a violation even though it's different from the default initialize in that it does accept an argument. I'm guessing the cop only looks at the method's body, not whether it takes arguments.


Expected behavior

Lint/UselessMethodDefinition would not mark an empty initialize method that takes arguments as a violation (or would at least offer configuration to mark such cases as valid).

Actual behavior

Lint/UselessMethodDefinition marks empty initialize methods that take arguments as a violation even though they differ from the default initialize by accepting arguments.

Steps to reproduce the problem

echo "class Foo
  def initialize(_unused_arg)
  end
end" > my_rubocop_issue.rb
bundle exec rubocop my_rubocop_issue.rb

RuboCop version

$ bundle exec rubocop -V
0.90.0 (using Parser 2.7.1.4, rubocop-ast 0.3.0, running on ruby 2.6.2 x86_64-darwin18)

Thanks!

@jtannas
Copy link

jtannas commented Sep 2, 2020

I've run into a similar issue using the following code:

class HttpBaseError < ::StandardError
  DEFAULT_MESSAGE = 'A server error has occured'
  def initialize(msg = self.class::DEFAULT_MESSAGE)
    super
  end
end

class BadRequest < HttpBaseError
  DEFAULT_MESSAGE = '400 BadRequest: Your request was not valid - please make corrections before trying again'
end
# etc...

ivgiuliani added a commit to gocardless/gc_ruboconfig that referenced this issue Sep 8, 2020
Basically every break is a false positives due to
rubocop/rubocop#8626
@jaredbeck
Copy link
Contributor

Here's another example, similar to @jtannas above, except with keyword arguments:

class MyClient < Client
  def initialize(token: MY_TOKEN)
    super
  end
end

class Client
  def initialize(options={})
    # .. uses options[:token] ..
  end
end
Offenses:

my_client.rb: W: Lint/UselessMethodDefinition: Useless method definition detected.
    def initialize(token: MY_TOKEN) ...

@marcandre
Copy link
Contributor

marcandre commented Sep 9, 2020

@jtannas & @jaredbeck: cases with default arguments (keyword or not) have been fixed by #8659.

I think the case of @andycandrea should not raise an issue (from this cop) as it doesn't call super. If it called super() it also should not raise an issue, only if it called super should it be really useless. @fatkodima what do you think?

@marcandre marcandre added bug and removed enhancement labels Sep 9, 2020
@schm
Copy link

schm commented Sep 10, 2020

In our code base, we had two occurrences of this warning, which were incorrect. We're dealing with a lot of duck typing, where we have many classes, that provide the same API, but are used in different contexts. E.g. we have noop classes, that can be used like their regular counter parts but they don't do anything.

class Base
  # not defining `initialize`, therefore depending on Object's default initialize w/o arguments
end

class RegularSub < Base
  def initialize(item)
    super()
    @item = item
  end
end

class NoopSub < Base
  def initialize(_item)
    super()
  end
end

class NoopAgain
  def initialize(_item); end
end

For both NoopSub#initialize and for NoopAgain#initialize, I get a warning. But I need to define initialize, because I want to divert from the default no-args constructor of Object.

Disclaimer: I'm fine with adding a # rubocop:disable tag for both cases in my code base. I just wanted to leave the examples here to provide some more context, if you decide to change the cop's behaviour somehow.

@marcandre
Copy link
Contributor

marcandre commented Sep 10, 2020

Right. As I stated, I think the case to be detected are single calls to super (with no parenthesis, no default values) or super(same, list, of, arguments, as, initialize). Others should not warn (like NoopSub#initialize and for NoopAgain#initialize)

Note: you will get an offense from another cop about not calling super at all in NoopAgain#initialize (which I feel is well deserved)

I imagine that @fatkodima will want to tweak this himself, but otherwise I'll do it :-)

@fatkodima
Copy link
Contributor

@marcandre Agreed with your points.

but otherwise I'll do it :-)

Yes, please, do it 😄

@schm
Copy link

schm commented Sep 10, 2020

Thanks for taking the time to look at the example and for providing feedback. It's great if these get changed.

Note: you will get an offense from another cop about not calling super at all in NoopAgain#initialize (which I feel is well deserved)

I don't think so as NoopAgain is inheriting from Object directly, i.e. we're just skipping Object's empty constructor. At least today, I'm not getting any such warning.

@marcandre
Copy link
Contributor

I don't think so as NoopAgain is inheriting from Object directly,

Oh right, I missed that.

marcandre added a commit to marcandre/rubocop that referenced this issue Sep 10, 2020
…ition`.

Note that not calling `super` at all from `initialize` is left
to `Lint/MissingSuper` cop.
@mergify mergify bot closed this as completed in 92d3fc6 Sep 10, 2020
ivgiuliani added a commit to gocardless/gc_ruboconfig that referenced this issue Oct 21, 2020
Basically every break is a false positives due to
rubocop/rubocop#8626
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
None yet
Development

No branches or pull requests

7 participants