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

Categorise rules and allow analysing only specific categories + new rule for changing ruleset to rulesets #278

Open
paulo-ferraz-oliveira opened this issue Feb 28, 2023 · 15 comments

Comments

@paulo-ferraz-oliveira
Copy link
Collaborator

Is your feature request related to a problem? Please describe

I'd like to be able to split elvis' rules into categories, have that information output in the result logs and be able to run only a specific set of categories.

e.g. refactor opportunity, good practice, readability

Describe the solution you'd like

No clear solution as of yet, but first we need to list the rules against the categories.

@paulo-ferraz-oliveira
Copy link
Collaborator Author

I'm thinking about how would we configure the categories that are applied? A new option alongside ruleset? And what would be the priority? Maybe we can actually use ruleset to extend the sets to be used as categories?

e.g. erl_files is the "catch-all" ruleset, and others would be stuff like software_design, code_readability, etc.

@elbrujohalcon
Copy link
Member

I like your idea but the I would change the ruleset param in the config to rulesets, so we can apply multiple rulesets to the same group of files.
Or I would adjust the custom ruleset definition to allow users to list rulesets when defining their own rulesets.
BTW… I think we lost the documentation about custom rulesets that was introduced in #141 to the wiki =/

@elbrujohalcon elbrujohalcon added this to the eventually 🙄 milestone Sep 16, 2024
@paulo-ferraz-oliveira
Copy link
Collaborator Author

paulo-ferraz-oliveira commented Sep 16, 2024

I like your idea but the I would change the ruleset param in the config to rulesets, so we can apply multiple rulesets to the same group of files.

Can you give an example?

Or I would adjust the custom ruleset definition to allow users to list rulesets when defining their own rulesets.

Or an example for this one?

BTW… I think we lost the documentation about custom rulesets that was introduced in #141 to the wiki =/

I don't think we've used a Wiki since we introduced the .md but I may be remembering wrong (in 2022). The .mds mention it but I don't know if the context is complete. Would the author or that PR have access to the Wiki in order to add/update stuff?

@elbrujohalcon
Copy link
Member

elbrujohalcon commented Sep 17, 2024

I like your idea but the I would change the ruleset param in the config to rulesets, so we can apply multiple rulesets to the same group of files.

Can you give an example?

[
    {elvis, [
        {config, [
            #{
                dirs => ["src"],
                filter => "*.erl",
                rulesets => [this_ruleset, the_other_ruleset, that_ruleset, …]
            }
        ]}
    ]}
].

Or I would adjust the custom ruleset definition to allow users to list rulesets when defining their own rulesets.

Or an example for this one?

[
    {elvis, [
        {rulesets,
            #{my_custom_ruleset =>
                [{elvis_style, no_tabs}, this_ruleset, the_other_ruleset, that_ruleset, …]}
        },
        {config, [
            #{
                dirs => ["src"],
                filter => "*.erl",
                ruleset => my_custom_ruleset
            }
        ]}
    ]}
].

BTW… I think we lost the documentation about custom rulesets that was introduced in #141 to the wiki =/

I don't think we've used a Wiki since we introduced the .md but I may be remembering wrong (in 2022). The .mds mention it but I don't know if the context is complete. Would the author or that PR have access to the Wiki in order to add/update stuff?

I guess they had… since we approved/merged the PR after asking them to update the wiki ;)

@paulo-ferraz-oliveira
Copy link
Collaborator Author

Ok, so if there's only one ruleset we coerce it rulesets => [<single_ruleset>] (?) and start proposing the new format (lest we break compatibility in a hard way here)?

In that case "categories" becomes simply "rulesets" and it'd play nice with the expected results summary I propose (good idea). Now we just have to find the proper rulesets :) I'll try to name them and put the rules in them...

As for the custom rulesets, I don't know if it'd apply. The goal is for us to be able to add more meta info. to the rules, and custom rulesets, as implemented, as already very flexible in that regard.

@paulo-ferraz-oliveira
Copy link
Collaborator Author

What d'ya think about this?

Category "Style and formatting"

Rules:

  • line_length
  • no_dollar_space
  • no_space_after_pound
  • no_tabs
  • no_trailing_whitespace
  • numeric_format

Category "Modularity and structure"

Rules:

  • god_modules
  • max_function_arity
  • max_function_length
  • max_module_length
  • nesting_level
  • no_successive_maps

Category "Readability"

Rules:

  • consistent_variable_casing
  • no_space
  • operator_spaces
  • prefer_unquoted_atoms

Category "Refactoring"

Rules:

  • max_anonymous_function_arity
  • no_match_in_condition
  • no_single_clause_case
  • param_pattern_matching

Category "Error-prone patterns"

Rules:

  • no_call
  • no_catch_expressions
  • no_common_caveats_call
  • no_debug_call
  • no_macros
  • no_throw
  • no_types

Category "Consistency"

Rules:

  • consistent_generic_type
  • no_spec_with_records
  • used_ignored_variable

Category "Naming"

Rules:

  • atom_naming_convention
  • behaviour_spelling
  • function_naming_convention
  • macro_module_names
  • macro_names
  • module_naming_convention
  • variable_naming_convention

Category "Anti-patterns"

Rules:

  • always_shortcircuit
  • dont_repeat_yourself
  • no_author
  • no_behavior_info
  • no_block_expressions
  • no_if_expression
  • no_import
  • no_nested_try_catch
  • no_specs

Category "Type and interface enforcement"

Rules:

  • export_used_types
  • invalid_dynamic_call
  • private_data_types
  • state_record_and_type

Category "Project"

Rules:

  • gitignore_patterns
  • no_branch_deps
  • no_deps_master_erlang_mk
  • no_deps_master_rebar
  • old_configuration_format
  • protocol_for_deps
  • protocol_for_deps_erlang_mk
  • protocol_for_deps_rebar

@elbrujohalcon
Copy link
Member

elbrujohalcon commented Sep 18, 2024

Ok, so if there's only one ruleset we coerce it rulesets => [<single_ruleset>] (?) and start proposing the new format (lest we break compatibility in a hard way here)?
Yeah, if we find ruleset => a_ruleset, we use rulesets => [a_ruleset].

And we add a rule to elvis_config to validate that it uses rulesets and not ruleset.

In that case "categories" becomes simply "rulesets" and it'd play nice with the expected results summary I propose (good idea). Now we just have to find the proper rulesets :) I'll try to name them and put the rules in them...

Exactly.

As for the custom rulesets, I don't know if it'd apply. The goal is for us to be able to add more meta info to the rules, and custom rulesets, as implemented, as already very flexible in that regard.

I don't understand this one, sorry. But I guess we can discuss adding rulesets to custom rulesets later. The important thing to verify is that Elvis should work fine if you add a custom ruleset within rulesets for any group of files. In other words, this should be valid: rulesets => [project, anti_patterns, my_custom_ruleset, …]. Initially, that's the only thing that matters.

@elbrujohalcon
Copy link
Member

Regarding the list… comments inline

Category "Style and formatting"

Rules:

  • line_length
  • no_dollar_space
  • no_space_after_pound
  • no_tabs
  • no_trailing_whitespace
  • numeric_format

I would add no_space and operator_spaces here.

Category "Modularity and structure"

Rules:

  • god_modules
  • max_function_arity
  • max_function_length
  • max_module_length
  • nesting_level
  • no_successive_maps

I think no_successive_maps is more of anti-pattern and I'm not sure where nesting_level should go but it doesn't look like the others to me.

Category "Readability"

Rules:

  • consistent_variable_casing
  • no_space
  • operator_spaces
  • prefer_unquoted_atoms

I think that consistent_variable_casing and prefer_unquoted_atoms should go wherever *_naming_convention rules are, or in the "Cosistency" category. no_space and operator_spaces belong to "style and formatting", in my opinion.

Category "Refactoring"

Rules:

  • max_anonymous_function_arity
  • no_match_in_condition
  • no_single_clause_case
  • param_pattern_matching

In my mind, max_anonymous_function_arity and max_function_arity should belong to the same category. And param_pattern_matching… is style-ish to me (it determines where you put the equal and the variable name on a match, it doesn't look like no_single_clause_case to me, but I'm not super sure about this).

Category "Error-prone patterns"

Rules:

  • no_call
  • no_catch_expressions
  • no_common_caveats_call
  • no_debug_call
  • no_macros
  • no_throw
  • no_types

To me, no_throw and no_if_expressons should belong together (they both are things that the language allows but you should not still avoid). But I'm not sure how to differentiate Error-prone patterns from Anti patterns, anyway. 🤔

Category "Consistency"

Rules:

  • consistent_generic_type
  • no_spec_with_records
  • used_ignored_variable

If we have a "Consistency" category… then that rule about using the word behaviour should be here… as well as consistent_variable_casing and possibly prefer_unquoted_atoms.

Category "Naming"

Rules:

  • atom_naming_convention
  • behaviour_spelling
  • function_naming_convention
  • macro_module_names
  • macro_names
  • module_naming_convention
  • variable_naming_convention

I like this one. I wouldn't mind having consistent_variable_casing here, but it may as well belong to the "Consistency" category, as I said before.

Category "Anti-patterns"

Rules:

  • always_shortcircuit
  • dont_repeat_yourself
  • no_author
  • no_behavior_info
  • no_block_expressions
  • no_if_expression
  • no_import
  • no_nested_try_catch
  • no_specs

As I said before, I would be inclined to merge this category with the "Error-prone" one above.

Category "Type and interface enforcement"

Rules:

  • export_used_types
  • invalid_dynamic_call
  • private_data_types
  • state_record_and_type

👌🏻

Category "Project"

Rules:

  • gitignore_patterns
  • no_branch_deps
  • no_deps_master_erlang_mk
  • no_deps_master_rebar
  • old_configuration_format
  • protocol_for_deps
  • protocol_for_deps_erlang_mk
  • protocol_for_deps_rebar

👌🏻

@elbrujohalcon
Copy link
Member

I did not check if you included all rules in the list above. In any case, we should also consider all the tickets for new rules that we have… and maybe think about the category each one should belong to.

@paulo-ferraz-oliveira
Copy link
Collaborator Author

and maybe think about the category each one should belong to.

Later, maybe?

I did not check if you included all rules in the list above

I did; I'll update the list with your comments and potentially comment some more.

@paulo-ferraz-oliveira
Copy link
Collaborator Author

As for the custom rulesets, I don't know if it'd apply. The goal is for us to be able to add more meta info to the rules, and custom rulesets, as implemented, as already very flexible in that regard.

I don't understand this one, sorry. But I guess we can discuss adding rulesets to custom rulesets later. The important thing to verify is that Elvis should work fine if you add a custom ruleset within rulesets for any group of files. In other words, this should be valid: rulesets => [project, anti_patterns, my_custom_ruleset, …]. Initially, that's the only thing that matters.

But I guess we can discuss adding rulesets to custom rulesets later

This.

@paulo-ferraz-oliveira
Copy link
Collaborator Author

And we add a rule to elvis_config to validate that it uses rulesets and not ruleset.

Enabled by default? If so, that's a breaking change, but I wouldn't mind, since we're kinda using SemVer already (and we have a migration helper).

@paulo-ferraz-oliveira
Copy link
Collaborator Author

The updated list is (categories "Readability" and "Anti-patterns" are gone)...

Category "Style and formatting"

Rules:

  • line_length
  • no_dollar_space
  • no_space_after_pound
  • no_tabs
  • no_trailing_whitespace
  • numeric_format
  • no_space ⚠️ moved here
  • operator_spaces ⚠️ moved here
  • param_pattern_matching ⚠️ moved here
  • nesting_level ⚠️ moved here

Category "Modularity and structure"

Rules:

  • god_modules
  • max_function_arity
  • max_function_length
  • max_module_length
  • max_anonymous_function_arity ⚠️ moved here

Category "Refactoring"

Rules:

  • no_match_in_condition
  • no_single_clause_case
  • param_pattern_matching

Category "Error-prone patterns"

Rules:

  • no_call
  • no_catch_expressions
  • no_common_caveats_call
  • no_debug_call
  • no_macros
  • no_throw
  • no_types
  • no_successive_maps ⚠️ moved here
  • always_shortcircuit ⚠️ moved here (was Anti-patterns)
  • dont_repeat_yourself ⚠️ moved here (was Anti-patterns)
  • no_author ⚠️ moved here (was Anti-patterns)
  • no_behavior_info ⚠️ moved here (was Anti-patterns)
  • no_block_expressions ⚠️ moved here (was Anti-patterns)
  • no_if_expression ⚠️ moved here (was Anti-patterns)
  • no_import ⚠️ moved here (was Anti-patterns)
  • no_nested_try_catch ⚠️ moved here (was Anti-patterns)
  • no_specs ⚠️ moved here (was Anti-patterns)

Category "Consistency"

Rules:

  • consistent_generic_type
  • no_spec_with_records
  • used_ignored_variable
  • behaviour_spelling ⚠️ moved here
  • consistent_variable_casing ⚠️ moved here
  • prefer_unquoted_atoms ⚠️ moved here

Category "Naming"

Rules:

  • atom_naming_convention
  • function_naming_convention
  • macro_module_names
  • macro_names
  • module_naming_convention
  • variable_naming_convention

Category "Type and interface enforcement"

Rules:

  • export_used_types
  • invalid_dynamic_call
  • private_data_types
  • state_record_and_type

Category "Project"

Rules:

  • gitignore_patterns
  • no_branch_deps
  • no_deps_master_erlang_mk
  • no_deps_master_rebar
  • old_configuration_format
  • protocol_for_deps
  • protocol_for_deps_erlang_mk
  • protocol_for_deps_rebar

@paulo-ferraz-oliveira paulo-ferraz-oliveira changed the title Categorise rules and allow analysing only specific categories Categorise rules and allow analysing only specific categories + new rule for changing ruleset to rulesets Sep 18, 2024
@paulo-ferraz-oliveira
Copy link
Collaborator Author

That new rule no_ruleset should go into Project, as should being-implement rules required_patterns and forbidden_patterns.

@elbrujohalcon
Copy link
Member

I ❤️ it!!

The final list is perfect!

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

No branches or pull requests

2 participants