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

Try to implement more M:F/A ignore rules #488

Closed
onno-vos-dev opened this issue Jul 3, 2018 · 8 comments · Fixed by inaka/elvis_core#137
Closed

Try to implement more M:F/A ignore rules #488

onno-vos-dev opened this issue Jul 3, 2018 · 8 comments · Fixed by inaka/elvis_core#137

Comments

@onno-vos-dev
Copy link
Contributor

In order to avoid marking an entire module to be ignored, I would like to explore the option of being able to ignore specific functions within a module, possibly including the arity as an optional value. The only rule that currently supports this behaviour is max_function_length_config and it would be great to support more rules in this way.

Especially on larger modules (one can argue that they shouldn't exist but reality sometimes punches us in the face), being forced to ignore the entire module because of one function that violates a specific rule makes me all sad.

Before exploring if this is feasible, I'd like to ask:

  1. Has this been considered and dropped due to feasibility reasons?
  2. Is this a feature that you would like to see implemented?
@elbrujohalcon
Copy link
Member

elbrujohalcon commented Jul 3, 2018

I ❤️ this idea and I would totally implement it with module attributes, just like dialyzer and xref (within rebar3 or xref_runner) do.
In other words, I would allow for individual modules to have some way to override the global config with attributes like:

-module my_module.

-elvis #{ module => [{elvis_style, god_modules, disable}]
        , {my_function, 2} => [{elvis_style, operator_spaces, #{rules => [{right, ","}]}}]
        , …
        }.

-export [my_function/2].

my_function(A, B) ->

That would be amazing!

@onno-vos-dev
Copy link
Contributor Author

I really like your second suggestion but that's a completely different beast altogether.

I can start looking into implementing my proposal. I suggest breaking off your 2nd approach to a different issue and we can track that seperately? 👍

@elbrujohalcon
Copy link
Member

elbrujohalcon commented Jul 3, 2018

Well… I'm not entirely sure what's your proposal, then.
Just add ignore clauses to several existing rules?
If that's so, then by all means go ahead!

@onno-vos-dev
Copy link
Contributor Author

Example config of what I mean

% -*- mode: erlang -*-
[{elvis,
  [{config,
    [ #{dirs => [ "lib/my_awesome_app/src"
                , "lib/my_legacy_app/src"
                ],
        filter => "*.erl",
        ruleset => erl_files,
        rules => [ {elvis_style, nesting_level,
                    #{ level => 3,
                       ignore_functions => [ {legacy_module, foo}
                                           , {legacy_module, bar, 2}
                                           ]
                     }}
                 ]}
    ]
   }
  ]
 }
].

Currently the nesting_level rule only allows ignoring a module. I'd like to be able to ignore legacy_module but only the function foo with any arity and bar with arity 2. In essence I want to bring the ignore_functions that is available functionality for the max_function_length_config-rule to more rules and allow a much more granular ignoring behavior than what is currently possible.

The problem that I'm trying to solve is implementing Elvis on a large codebase where Elvis is unhappy about a handful of functions within modules and want to avoid having to mark the entire module as a ignore. I want to start by adding these functions as ignore_functions and having the rest of the module be checked. This way, no more style issues can sneak into the code base and combined with https://github.com/inaka/elvis/issues/487 slowly but surely remove the style issues and get Elvis to dance happily-ever-after.

Does that clarify this?

@elbrujohalcon
Copy link
Member

It does clarify and yes… by all means, go ahead!!

@paulo-ferraz-oliveira
Copy link
Collaborator

I vote that we slightly change ignore to encompass ignore_functions and then add that to all the rules.

ignore could be :: module() | {module(), function()} | {module() | function() | arity()}

@elbrujohalcon: at the same time, it seems this Issue might have been closed by a previous PR already. I'll later check if a new Issue should be created with my "proposal" (I'm imagining this isn't a new suggestion, in any case).

@elbrujohalcon
Copy link
Member

@paulo-ferraz-oliveira I like your idea.
Besides… I still like the -elvis attribute 😎

@paulo-ferraz-oliveira
Copy link
Collaborator

Also partially mentioned in #507.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging a pull request may close this issue.

4 participants