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

Decrease chance of error and improve maintenance #135

Merged
merged 8 commits into from
Sep 18, 2020
Merged

Decrease chance of error and improve maintenance #135

merged 8 commits into from
Sep 18, 2020

Conversation

paulo-ferraz-oliveira
Copy link
Collaborator

I found different defaults in elvis_style and elvis_rulesets, and in those cases the public Wiki won.

[Fix inaka/elvis#525].

We found different defaults in elvis_style and elvis_rulesets, and in those cases the public
Wiki won
@paulo-ferraz-oliveira
Copy link
Collaborator Author

This isn't yet complete, but if the general direction isn't this one, I'll want to adapt before I move to the other rulesets.

@paulo-ferraz-oliveira
Copy link
Collaborator Author

paulo-ferraz-oliveira commented Sep 18, 2020

Anyway, I had a local version that used macros for functions default/2 and option. It simplified stuff, but may not be what you wanted (I also find it borderline convoluted).

Basically, where you have

option(OptionName, RuleConfig, Rule)

you'd have

?OPT(OptionName)

with

-define(OTP(OptionName), (fun () -> maps:get(OptionName, RuleConfig, default(?FUNCTION_NAME, OptionName)) end))

(with the assumption that RuleConfig was defined in the call context - I know, "dirty").

@paulo-ferraz-oliveira
Copy link
Collaborator Author

I then added a further change where default(?FUNCTION_NAME, OptionName) would also become

-define(?DEF(OptionName), OptionName => default(?FUNCTION_NAME, OptionName)).
% so we could build the options maps as:
#{ ?DEF(limit) },

@paulo-ferraz-oliveira
Copy link
Collaborator Author

Why these (local) changes?

I just feel that defining stuff as

    #{ limit => default(god_modules, limit)
     , ignore => default(god_modules, ignore)

is more error-prone than (though the advantage is there's no "magic")

    #{ ?DEF(limit)
     , ?DEF(ignore) }

and also I'd prefer to reduce stuff like

    Regex = option(regex, RuleConfig, function_naming_convention),
    Ignores = option(ignore, RuleConfig, function_naming_convention),

to

    Regex = ?OPT(regex),
    Ignores = ?OPT(ignore),

thus removing the need to keep using RuleConfig and the function name (or ?FUNCTION_NAME).

@paulo-ferraz-oliveira
Copy link
Collaborator Author

I pushed a preparation commit for those (macro-based) changes should we want them in elvis_style.

src/elvis_rulesets.erl Outdated Show resolved Hide resolved
src/elvis_style.erl Outdated Show resolved Hide resolved
src/elvis_style.erl Outdated Show resolved Hide resolved
src/elvis_style.erl Outdated Show resolved Hide resolved
@elbrujohalcon
Copy link
Member

Oh, and please move us to meck 0.9.0 🙏

@elbrujohalcon
Copy link
Member

Sorry, @paulo-ferraz-oliveira … but we have a rule against macros.
I'm inclined to reluctantly allow ?FUNCTION_NAME… although I would not use that myself. But that's like… the farthest I'm willing to go in that direction.

src/elvis_style.erl Outdated Show resolved Hide resolved
@paulo-ferraz-oliveira
Copy link
Collaborator Author

paulo-ferraz-oliveira commented Sep 18, 2020

Oh, and please move us to meck 0.9.0 🙏

I didn't want to change it, since it wasn't affecting the scope of my PR. But I'll do it, as per your request.

Edit: 317c0d5

@paulo-ferraz-oliveira
Copy link
Collaborator Author

paulo-ferraz-oliveira commented Sep 18, 2020

Sorry, @paulo-ferraz-oliveira … but we have a rule against macros.
I'm inclined to reluctantly allow ?FUNCTION_NAME… although I would not use that myself. But that's like… the farthest I'm willing to go in that direction.

Hm... I guess it never made it to elvis. It is proposed here though. We'll eventually get there :)

Edit: also wanted to point out that elvis already has quite a few user-defined macros; and not that's it's "better", but ?FUNCTION_NAME is built-in (since Erlang/OTP 19, if I'm not mistaken).

Edit: 59300dd

@paulo-ferraz-oliveira
Copy link
Collaborator Author

paulo-ferraz-oliveira commented Sep 18, 2020

@elbrujohalcon, don't merge just yet; there's stuff missing for elvis_project. (I just wanted to understand if I was going in the right direction)

Edit: a7f57b8

…inciples everywhere

From elvis_style to elvis_project
Copy link
Member

@elbrujohalcon elbrujohalcon left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Is it ready now, @paulo-ferraz-oliveira ?

@paulo-ferraz-oliveira
Copy link
Collaborator Author

It is.

@elbrujohalcon elbrujohalcon merged commit bcb9db5 into inaka:master Sep 18, 2020
@paulo-ferraz-oliveira paulo-ferraz-oliveira deleted the feature/same_source_defaults branch September 28, 2020 21:36
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Use of defaults in code
2 participants