Open paulo-ferraz-oliveira opened 1 year ago
paulo-ferraz-oliveira
commented
1 month ago 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
commented
1 month ago 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 =/
paulo-ferraz-oliveira
commented
1 month ago 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 https://github.com/inaka/elvis_core/pull/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
commented
1 month ago 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
.mdbut 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
commented
1 month ago 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
commented
1 month ago What d'ya think about this?
Rules:
Rules:
Rules:
Rules:
Rules:
Rules:
Rules:
Rules:
Rules:
Rules:
elbrujohalcon
commented
1 month ago 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 findruleset => a_ruleset, we userulesets => [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
commented
1 month ago 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
commented
1 month ago 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
commented
1 month ago 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
commented
1 month ago 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
commented
1 month ago 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
commented
1 month ago The updated list is (categories "Readability" and "Anti-patterns" are gone)...
Rules:
Rules:
Rules:
Rules:
Rules:
Rules:
Rules:
Rules:
paulo-ferraz-oliveira
commented
1 month ago That new rule no_ruleset should go into Project, as should being-implement rules required_patterns and forbidden_patterns.
elbrujohalcon
commented
1 month ago The final list is perfect!
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.