makandra / makandra-rubocop

makandra's default Rubocop configuration
MIT License
6 stars 1 forks source link

Unused method argument in abstract method #2

Closed dastra-mak closed 5 years ago

dastra-mak commented 5 years ago

I noticed that rubocop will try to prefix unused method arguments with underscores. In general I agree with this, but I found a case in which I don't want the prefix. This is with abstract methods which should just define the signature of a method.

-        def pick_value(record)
+        def pick_value(_record)
           raise NotImplementedError
         end

Should we just accept the fact that the arguments are prefixed in this situation or disable the cop?

https://www.rubydoc.info/gems/rubocop/RuboCop/Cop/Lint/UnusedMethodArgument

triskweline commented 5 years ago

I think if we're going to prefix unused args, it's OK to also have them prefixed in this example. After all, they're unused in the interface definition.

BTW Rubocop's style guide is against abstract methods, but I think they are very valuable for communicating an intended interface.

foobear commented 5 years ago

Ruby will print warnings (when enabled) for unused local variables, unless they are prefixed with an underscore. I agree with @triskweline: if we want to prefix those, unused method arguments should also be prefixed for consistency (even though Ruby won't warn for them).

I don't consider difference in argument names an issue. When overwriting methods, arguments do not need to have the same names as the super method.

Also agreed on the point of abstract methods and making an intended API clearly visible (though it's outside the scope here).

judithroth commented 5 years ago

We have not used the prefix with underscore for unused variables in our projects yet. For me it does not add value, because my editor highlighting shows unused variables very clearly. So I vote for disabling the cop.

kratob commented 5 years ago

Vote for "keep enabled", especially since Ruby and some IDEs show unused variables, and I'd rather not see unwarranted warnings.