Use of defaults in code

[paulo-ferraz-oliveira]

Due to the fact that the defaults in elvis_rulesets.erl and elvis_style.erl aren't always the same, the following might happen.

My elvis.config file has {elvis_style, line_length, #{}}. If I look at the Wiki, I might (wrongly assume the default is 100). The default being applied, in this case, isn't coming from the rulesets but from the style, since the option map is present but the key isn't, which means maps:get(limit, RuleConfig, 80) evaluates to 80, not 100 (as expected).

Do you think it's best to:

1. leave this matter alone (it's fine as it is),
2. the defaults should be removed from the rulesets and only exist in the style (the rulesets would just include the rules themselves, not the options),
3. the defaults should be e.g. exposed in rulesets and then imported in style as, for example,

% rulesets.erl
...
-export([default_line_length_limit/0]).
-define(DEFAULT_LINE_LENGTH_LIMIT, 100).
...
default_line_length_limit() -> ?DEFAULT_LINE_LENGTH_LIMIT.
...

% style.erl
...
line_length(_Config, Target, RuleConfig) ->
    Limit = maps:get(limit, RuleConfig, elvis_rulesets:default_line_length_limit()),
    ...

?

[elbrujohalcon]

Your concern is very valid.
I would personally prefer your option #3, as long as you don't use macros and instead return the default value directly in the functions… Even more, I would do it this way:

-spec default(module(), atom(), atom()) -> term().
default(elvis_style, line_length, limit) -> 100;
default(elvis_style, line_length, skip_comments) -> false;
…

…or even…

-spec default(module(), atom()) -> #{atom() => term()}.
default(elvis_style, line_length) -> #{limit => 100, skip_comments => false};
…

So that it can be used directly in the rulesets.

[paulo-ferraz-oliveira]

Thanks, @elbrujohalcon. I think I'll go with a mix of both. A function that allows for an easier to maintain elvis_rulesets and a function with the defaults, so that elvis_rule can be easily maintained to. Both will read from the same source.

[paulo-ferraz-oliveira]

Something like

-spec default(module(), function(), Option :: atom()) -> DefaultValue :: term().
default(elvis_style, line_length, limit) -> 100;
default(elvis_style, line_length, skip_comments) -> false.

-spec default(module(), function()) -> DefaultOption :: term().
default(elvis_style = M, line_length = F) -> #{limit => default(M, F, limit),
                                               skip_comments => default(M, F, limit)}.

, perhaps?

[paulo-ferraz-oliveira]

maps:get(skip_comments, RuleConfig, false)

would become a more generic

get_option(M, F, Option, RuleConfig) ->
    maps:get(Option, RuleConfig, default(M, F, Option)).

or is this too abstract?

[paulo-ferraz-oliveira]

The way I'm looking at this now, which thus implied changes to two test cases, is that:

• if a rule map is #{} it doesn't mean "apply no options", since for that we can use disabled,
• thus, it actually means "I have no defined options, use the defaults".

[elbrujohalcon]

I don't really like having 2 default functions… Someday someone will add a literal value to a map default/1 without using default/3 and we'll then have a mix of places to put the defaults.
I would personally choose to only have default/1 which returns both the maps with the default list of options and the defaults for those options… all written down in just one place.

[elbrujohalcon]

The way I'm looking at this now, which thus implied changes to two test cases, is that:

• if a rule map is #{} it doesn't mean "apply no options", since for that we can use disabled,
• thus, it actually means "I have no defined options, use the defaults".

I like this but it has to be properly documented.

[paulo-ferraz-oliveira]

The way I'm looking at this now, which thus implied changes to two test cases, is that:

• if a rule map is #{} it doesn't mean "apply no options", since for that we can use disabled,
• thus, it actually means "I have no defined options, use the defaults".

I like this but it has to be properly documented.

Where should it be documented?

[paulo-ferraz-oliveira]

I don't really like having 2 default functions… Someday someone will add a literal value to a map default/1 without using default/3 and we'll then have a mix of places to put the defaults.
I would personally choose to only have default/1 which returns both the maps with the default list of options and the defaults for those options… all written down in just one place.

Sure, will do. It's an extra maps:get I think, but it should be performance-degrading in a significant way.

[elbrujohalcon]

Sure, will do. It's an extra maps:get I think, but it should be performance-degrading in a significant way.

It shouldn't be performance degrading.

Where should it be documented?

Probably here.

[paulo-ferraz-oliveira]

Should this be enough:

Note: using #{}, as an option map, will mean that defaults apply, i.e. it's not the same as disable
?

[elbrujohalcon]

Yeah!

[paulo-ferraz-oliveira]

Yeah!

Done!