Add prefer-number-is-integer rule
#1509
Conversation
rules/prefer-number-is-integer.js
Outdated
| '[left.callee.type="MemberExpression"]', | ||
| '[left.callee.object.name="Math"]', | ||
| '[left.callee.property.name="round"]', | ||
| '[left.arguments.0.type="Identifier"]', |
There was a problem hiding this comment.
I don't think we should limit this to Identifier, it should work for anything, just make sure the argument and the right side are the same reference, you can use isSameReference.
And don't auto-fix if the expression has side effect.
There was a problem hiding this comment.
@fisker I've updated the PR to use isSameReference.
Can you give me an example of an expression with a side effect that fits here? So I can TDD against it :)
|
Thanks for the feedback on PR, I'll get onto updating this PR over the next week :) |
tristanHessell
force-pushed
the
add-prefer-number-is-integer
branch
from
2367c8f
to
2e39762
Compare
|
Sorry for the long delay, overall looks good. But I'm not sure if we want check Can you fix tests? |
|
Happy to make the changes! Will get it done over the next week or so |
rules/prefer-number-is-integer.js
Outdated
| equalsSelector, | ||
| ].join(''); | ||
|
|
||
| // ParseInt(value,10) === value |
There was a problem hiding this comment.
| // ParseInt(value,10) === value | |
| // `Number.parseInt(value, 10) === value` |
Let's forget about parseInt, only check Number.parseInt
rules/prefer-number-is-integer.js
Outdated
| equalsSelector, | ||
| ].join(''); | ||
|
|
||
| // _.isInteger(value) |
There was a problem hiding this comment.
Use methodCallSelector, search how to use it in other rules.
rules/prefer-number-is-integer.js
Outdated
| '[callee.property.name="isInteger"]', | ||
| ].join(''); | ||
|
|
||
| // Math.round(value) === value |
|
Bump :) |
|
@sindresorhus appreciated! I had forgot about this PR. Working on it over the next couple of days |
tristanHessell
force-pushed
the
add-prefer-number-is-integer
branch
from
2e39762
to
6b4f721
Compare
|
|
||
| default: { | ||
| return ''; | ||
| } |
There was a problem hiding this comment.
@sindresorhus what should we do here?
I've added the default case as to appease linting, but the calling code protects us from this branch being taken.
How should I approach appeasing Codecov?
There was a problem hiding this comment.
You could use an ignore comment:
/* c8 ignore next 3 */
|
@sindresorhus et al, this is ready or review :) |
| return node => { | ||
| const variable = variableGetter(node); | ||
|
|
||
| if (!variable) { |
There was a problem hiding this comment.
We should not limit this to Identifier and MemberExpression, should allow anything
_.isInteger(a ? b : c)
|
@tristanHessell Bump |
|
Closing for lack of response. |
Fixes #1460.
One thing to note is that I couldn't run
npm run integrationto completion as my computer only has 4GB RAM.