Metrics/ParameterLists supports MaxOptionalParameters config parameter

[fatkodima]

This is an implementation of https://rubystyle.guide/#keyword-arguments-vs-optional-arguments

Not sure if something like IgnoredMethods should be added - like for this https://github.com/rubocop-hq/rubocop/compare/master...fatkodima:too_many_default_arguments-cop?expand=1#diff-3b2d7d0e5589b3568a72de6447fa1095b2e9bae437f299ab43196979d0d50c01R36

[bbatsov]

I think the real problem is not a particular number of default arguments, but having any of them in the first place, and mixing them with keyword arguments. I'd just discourage their usage completely, but that's just me. @rubocop-hq/rubocop-core wha t do you think?

[marcandre]

There already a cop for too many positional arguments I believe, but the default maximum is super high (5?).

My personal preference:

• public methods: max of 1 positional or rest argument, everything else is keyword argument ("Rails convention")
• private methods: no maximum, but never any default for positional arguments. It's typically to pass some context around, so typically the arguments passed will be clear on the call site.

[fatkodima]

My intention with this cop was to avoid ugliness like

def do_something(a = nil, b = nil, c = nil, d = nil)
end

do_something(nil, nil, nil, value)

and more general

def do_something(a = 1, b = 2, c = 3)
end

do_something(1, 2, 4) # you have to repeat default values for "a" and "b" to call with different "c"

but having any of them in the first place, and mixing them with keyword arguments

agreed with mixing

Feel free to close this if is not worth it.

[bbatsov]

Btw, I was just reminded that we already have a cop Style/OptionalArguments (yeah, I know we messed up its name), and it seems natural to me to just add the limit for optional arguments there.

[bbatsov]

As for keyword vs optional params - I'd love for us to have some cop that enforces more or less what @marcandre suggested, but I think that's orthogonal to @fatkodima's current suggestion.

[fatkodima]

and it seems natural to me to just add the limit for optional arguments there.

This is what I wanted 👍 So added that config.

[bbatsov]

argument -> parameter

[bbatsov]

Btw, I was thinking that we can also have this check in the other cops Metrics/ParameterLists. I'm on the fence which method is a better home for it.

[fatkodima]

Forgot about its existence. Metrics/ParameterLists seems a better place to me, since the original cop deals with default parameters placement (at the end or not), but the mentioned one with counts of parameters.

[bbatsov]

Okay, let's go for it then. Sorry that I'm making you do so much extra work, I just have trouble remembering everything we've done so far. 😆

[fatkodima]

No worries. I have a lot of free time 😄