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
no-magic-number
issues
#4616
Comments
I'm going to take a wild guess here about why those tests are passing, but I have no real insider knowledge here. I don't think the point of the rule is to avoid assigning numbers to variables. In fact, the point of the rule is to force numbers to be assigned to variables, rather than be used directly in expressions throughout the code. I don't know what
|
Ok, you are absolutely right about those tests. I lost the forest behind the trees, and it makes sense that assigning value to a variable should pass. So those unittests are passing correctly. Based on the code, however, I'm not sure why is |
@kombucha any insights? |
These seem like they shouldn't be valid: var min, max, mean; min = 1; max = 10; mean = 4;
var a = {}; a[9] = 'test'; The intent of var foo = {
bar: 5
}; I agree, this is confusing. We should figure out a way to clean this up. |
@ilyavolodin any followup on this? |
I'm not really sure what to do about those issues. I didn't participate in the original discussion and don't know the reasoning behind some of the decisions for this rule. Since we are not getting any response from the author, I think we basically have to re-write this rule and clean-up documentation and tests. |
What do we want to do here?
This is not a valid test case, @ilyavolodin I think you might have gotten the behaviour backwards. @nzakas example is the correct one of what this is supposed to flag. So, I think the only issue is if we want this rule to flag or not:
|
I think we do. |
Accepting this as a bug, then. |
Working on this. |
I was working on fix for #4370 and I got very confused by the logic of this rule. Fix itself is easy to add, but I don't understand how some of the unittests are passing and I also don't understand what
detectObjects
supposed to do.detectObjects
is very poorly tested and doesn't have a single valid test case. It's also clearly incorrect (as it will ignore any number who's parent has a type ofAssignmentExpression
which is the whole point of this rule). Can somebody explain whatdetectObjects
supposed to do and why are the following unit tests passing:https://github.com/eslint/eslint/blob/master/tests/lib/rules/no-magic-numbers.js#L34
https://github.com/eslint/eslint/blob/master/tests/lib/rules/no-magic-numbers.js#L38
https://github.com/eslint/eslint/blob/master/tests/lib/rules/no-magic-numbers.js#L45
https://github.com/eslint/eslint/blob/master/tests/lib/rules/no-magic-numbers.js#L60
https://github.com/eslint/eslint/blob/master/tests/lib/rules/no-magic-numbers.js#L63
Want to back this issue? Post a bounty on it! We accept bounties via Bountysource.
The text was updated successfully, but these errors were encountered: