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

Uncommunicative{MethodArg,BlockParam}Name fails for * #5436

Closed
denisdefreyne opened this issue Jan 11, 2018 · 13 comments
Closed

Uncommunicative{MethodArg,BlockParam}Name fails for * #5436

denisdefreyne opened this issue Jan 11, 2018 · 13 comments

Comments

@denisdefreyne
Copy link

denisdefreyne commented Jan 11, 2018

Expected behavior

UncommunicativeMethodArgName and UncommunicativeBlockParamName should not trigger when given * as an argument.

Actual behavior

UncommunicativeMethodArgName and UncommunicativeBlockParamName treat a * argument as an offense.

Steps to reproduce the problem

Create a file with contents:

def stuff(*); end

Run Rubocop:

moo.rb:3:11: C: Naming/UncommunicativeMethodArgName: Method argument must be longer than 3 characters.
def stuff(*); end

RuboCop version

4c15154 from Git (past 0.52.1)

@denisdefreyne denisdefreyne changed the title UncommunicativeMethodArgName fails for * Uncommunicative{Method,Block}ArgName fails for * Jan 11, 2018
@denisdefreyne denisdefreyne changed the title Uncommunicative{Method,Block}ArgName fails for * Uncommunicative{MethodArg,BlockParam}Name fails for * Jan 11, 2018
@denisdefreyne
Copy link
Author

The name of the two cops is inconsistent, too:

UncommunicativeMethodArgName
UncommunicativeBlockParamName

Arg vs Param

@bbatsov
Copy link
Collaborator

bbatsov commented Jan 11, 2018

Maybe we should add an exception about this indeed. But I recall we discussed with others that something like *args is arguably a better name. I know that 9/10 of Rubyists are confused by a naked * and don't know the only use-case for this is to pass the args to super.

Perhaps we can have a different cop to check for this though. Not sure at this point.

@bbatsov
Copy link
Collaborator

bbatsov commented Jan 11, 2018

Arg vs Param

Good catch. Should be Param in both cases. //cc @garettarrowood

@denisdefreyne
Copy link
Author

But I recall we discussed with others that something like *args is arguably a better name.

That makes sense.

I recall Rubocop at some point in the past suggesting me to change unused args from *args to *_args and then to *, which I think is the reason why I ended up with the latter.

@denisdefreyne
Copy link
Author

don't know the only use-case for this is to pass the args to super.

TIL that you can still pass args to super this way. Point proven!

@denisdefreyne
Copy link
Author

Ah, here we go:

class Thing
  def run(*args)
    raise NotImplementedError
  end
end
W: Lint/UnusedMethodArgument: Unused method argument - args. If it's necessary, use _ or _args as an argument name to indicate that it won't be used. You can also write as run(*) if you want the method to accept any arguments but don't care about them.
  def run(*args)
           ^^^^

@bbatsov
Copy link
Collaborator

bbatsov commented Jan 11, 2018

I recall Rubocop at some point in the past suggesting me to change unused args from *args to *_args and then to *, which I think is the reason why I ended up with the latter.

Ah, yeah. Now I remembered this as well. Personally I always used *_args in such situations, as it seemed way more descriptive to me. I guess for some people it's too verbose, though. Anyways, I'm completely open to whitelisting this, even if I think it's not very communicative in general.

@Drenmi
Copy link
Collaborator

Drenmi commented Jan 11, 2018

At the moment, parameters are (incorrectly) referred to as arguments in most of the codebase I think. Perhaps that needs a more sweeping refactoring if it is becoming a problem.

@garettarrowood
Copy link
Contributor

Could someone point me towards an explanation why argument is wrong here? Besides it being all over this codebase, its also referred to method arguments all over Google.

This is not an argument for/against a change. I'm just wondering where the source of truth is with method parameter/argument terminology.

@denisdefreyne
Copy link
Author

Could someone point me towards an explanation why argument is wrong here?

As I understand it, a parameter is the name of the variable defined in the method, so thing in the example below is a parameter (not an argument).

def stuff(thing); end

An argument is the value that you pass to a function, so "hi" is an argument (not a parameter):

stuff("hi")

See e.g. this StackOverflow post for some more discussion on the subject.

@denisdefreyne
Copy link
Author

The distinction becomes a little vague in this situation:

def run(*arguments); end

arguments is a parameter (not an argument), but I believe that the name is still correct; the arguments parameter after all contains the list of all arguments.

@garettarrowood
Copy link
Contributor

Thanks for the clarification here! I'll change the name of the new cop.

@marcandre
Copy link
Contributor

Does this also handle **?

This was referenced Mar 21, 2018
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

No branches or pull requests

5 participants