feat: noMagicNumbers #4971
feat: noMagicNumbers #4971
Conversation
github-actions
bot
added
A-Project
|
Hey @Conaclos, @ematipico This is an initial draft for the implementation of the no-magic-numbers lint rule. My knowledge of Rust is limited. So, I'd like to get some early feedback here. P.S. Please note that I used GitHub Copilot to write this code |
|
As you might notice, the lint job already gives you some diagnostics. You can start by fixing them. |
| /// let total = price * TAX_RATE; | ||
| /// ``` | ||
| pub NoMagicNumbers { | ||
| version: "1.0.0", |
There was a problem hiding this comment.
| version: "1.0.0", | |
| version: "next", |
| language: "js", | ||
| sources: &[RuleSource::Eslint("no-magic-numbers")], | ||
| recommended: false, | ||
| fix_kind: FixKind::None, |
There was a problem hiding this comment.
| fix_kind: FixKind::None, |
This isn't needed
| @@ -0,0 +1,2 @@ | |||
| /* should not generate diagnostics */ | |||
| var a = 1; | |||
There was a problem hiding this comment.
The scaffolding should already provide a comment saying that code here shouldn't generate diagnostics, but the snapshot test did. Can you fix it?
| detect_objects: bool, | ||
| enforce_const: bool, | ||
| ignore: Vec<i64>, | ||
| ignore_array_indexes: bool, | ||
| ignore_default_values: bool, | ||
| ignore_class_field_initial_values: bool, |
There was a problem hiding this comment.
Each field must be documented. These options will appear in the configuration schema, so we should provide a description for our users.
|
|
||
| fn run(ctx: &RuleContext<Self>) -> Self::Signals { | ||
| let node = ctx.query(); | ||
| let config = ctx.options(); |
| range: literal.range(), | ||
| raw: value_token.text().to_string(), |
There was a problem hiding this comment.
Pass the literal node instead, and then extract the range and the name inside the diagnostic function. It will avoid allocating a String
| let diagnostic = if state.is_const_violation { | ||
| RuleDiagnostic::new( | ||
| rule_category!(), | ||
| state.range, | ||
| markup! { "Number constants declarations must use 'const'" }, | ||
| ) | ||
| } else { | ||
| RuleDiagnostic::new( | ||
| rule_category!(), | ||
| state.range, | ||
| markup! { "No magic number: "{state.raw} }, | ||
| ) | ||
| .note(markup! { | ||
| "Consider extracting this magic number into a named constant." | ||
| }) | ||
| }; | ||
| Some(diagnostic) |
There was a problem hiding this comment.
| let diagnostic = if state.is_const_violation { | |
| RuleDiagnostic::new( | |
| rule_category!(), | |
| state.range, | |
| markup! { "Number constants declarations must use 'const'" }, | |
| ) | |
| } else { | |
| RuleDiagnostic::new( | |
| rule_category!(), | |
| state.range, | |
| markup! { "No magic number: "{state.raw} }, | |
| ) | |
| .note(markup! { | |
| "Consider extracting this magic number into a named constant." | |
| }) | |
| }; | |
| Some(diagnostic) | |
| if state.is_const_violation { | |
| RuleDiagnostic::new( | |
| rule_category!(), | |
| state.range, | |
| markup! { "Number constants declarations must use 'const'" }, | |
| ) | |
| } else { | |
| RuleDiagnostic::new( | |
| rule_category!(), | |
| state.range, | |
| markup! { "No magic number: "{state.raw} }, | |
| ) | |
| .note(markup! { | |
| "Consider extracting this magic number into a named constant." | |
| }) | |
| } |
| } | ||
| } | ||
|
|
||
| fn is_allowed_number( |
There was a problem hiding this comment.
Remove the commented code and add some documentation to the function. We want to explain the business logic of "when a number is allowed"
| false | ||
| } | ||
|
|
||
| fn check_const_violation( |
There was a problem hiding this comment.
There's no documentation about this "const violation" concept. You might want to add it somewhere, even in the official documentation of the rule
| .and_then(|decl| decl.syntax().parent()) | ||
| .and_then(JsVariableDeclarator::cast) | ||
| .and_then(|parent| parent.id().ok()) | ||
| .map_or(false, |kind| kind.to_string() != "const") |
There was a problem hiding this comment.
Don't convert to String when you just need a &str. Look for text_trimmed method
Summary
implements #4333 no-magic-numbers from ESlint https://eslint.org/docs/latest/rules/no-magic-numbers
Test Plan
Tests included