[Rule] Too many arguments without default

[Lakitna]

I am personally not a fan of the rule too-many-arguments. Instead, I want to propose too-many-arguments-without-default.

A lot of arguments can make a keyword cumbersome to use. It can also be an indication of too much keyword complexity. However, I don't think having a lot of arguments with defaults is always an issue. For example:

*** Keywords ***
Create user account
    [Arguments]
    ...  ${environment}=tst
    ...  ${type}=online
    ...  ${validateAge}=${True}
    ...  ${validateEmail}=${True}
    ...  ${acceptTerms}=${True}
    ...  ${retentionDays}=${14}

This is (a modified version of) a keyword we use all the time for user account creation. Note it has more than the default allowed 5 arguments, but all have a default value. The unmodified version has 10 arguments, all with a sensible default value.

In most tests, we use the keyword without arguments. In tests where we need a specific account state, we use the keyword with a few arguments overwritten. I think that with a pattern like this, you can create great keywords with many arguments. These keywords offer great flexibility.

At the same time, I think that the following is a bad pattern and should not be used:

*** Keywords ***
Create user account
    [Arguments]
    ...  ${environment}
    ...  ${type}
    ...  ${validateAge}
    ...  ${validateEmail}
    ...  ${acceptTerms}
    ...  ${retentionDays}=${14}

There are too many arguments here that you always have to explicitly fill. This makes the keyword hard to use and hard to understand.

There are a few ways we can achieve this:

1. Add the new rule too-many-arguments-without-default as a non-default rule.
2. Add a new parameter to the existing rule too-many-arguments. This parameter could be called ignore_optional_arguments.
3. Make this the new behavior of the existing rule too-many-arguments.

If you agree with me, please let me know and I'll open a PR.

[bhirsz]

That's good idea. For how we want to do it, I would:

• cross out 3. option. Some people may want to still use general too-many-arguments rule

I'm debating between 1 and 2. If we want to check number of default arguments separately, we might also want to parametrize it (ie. allow up to 10 default arguments, no more). too-many-arguments-without-default focus on non-default arguments. And currently too-many-arguments counts all arguments, so configuring 'max_args' applies to both types. There is no option to only check number of default arguments.

Given that, maybe:

too-many-arguments would not change and still count all arguments (default and non-default)
new too-many-arguments-without-default rule for counting non-default ones (parametrizable)
and new too-many-arguments-with-default rule for counting default ones (parametrizable)?

Both new rules would be non-default.

[Lakitna]

I like the three-rule approach. It offers maximum flexibility in an understandable way. We can also name the new rules too-many-required-arguments and too-many-optional-arguments. However, the latter is not as clear.

I'm not so sure about the defaults though. I think it might be worth making too-many-arguments optional in favor of the new ones. For example something like:

• too-many-arguments disabled max_args=5
• too-many-required-arguments enabled max_args=3
• too-many-optional-arguments enabled max_args=10

This is a breaking change, but I think it'd be worth it during a major release.

[bhirsz]

Agree on required/optional names.

3 required arguments can be bit too low, if we stick to 5 it would be breaking change but with minimal impact (nothing will fail as people with existing default configuration will use 5. If they configure max_args in too-many-arguments, then they will need to do the same for new rules).

[Lakitna]

I took a good look at our codebase to see what a reasonable default could be. I agree, 3 is a bit too low, but I also think that 5 is too high. In our codebase, it looks like 4 is the sweet spot.

With max 4 required arguments, I may have to add a couple of disablers, but most keywords with 5 or more should be solved in a different way. In our way of working, the only bar you have to clear to justify a disabler is to convince the reviewer. This allows us to be a bit stricter with our lint rules than others might want.

With that in mind, I'd change max_args for our codebase to 4, but I can also support making it 5 by default. This would give a little bit of headroom for those who dislike disablers.

[bhirsz]

Having keywords in test with 5 arguments does sound excessive - but I'm sticking to it since it's current default. If the official code style mention it, I will adjust the numbers accordingly. Thanks for checking out the numbers