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

Add prefer-math-trunc rule #851

Merged
merged 14 commits into from
Oct 6, 2020
Merged

Add prefer-math-trunc rule #851

merged 14 commits into from
Oct 6, 2020

Conversation

noftaly
Copy link
Contributor

@noftaly noftaly commented Oct 4, 2020

Added prefer-math-trunc rule, which forbids the use of | 0 over Math.trunc() to truncate numbers.
One thing i'm not really sure about is the error message. It says not to use | 0 to truncate numbers, but maybe it should say no to use bitwise OR with 0? Idk, let me know what you think of.

Fixes #827

@noftaly
Copy link
Contributor Author

noftaly commented Oct 4, 2020

I have a question: What to do with variables (or member expressions)? The fix can break one's code because undefined | 0 gives 0, but Math.trunc(undefined) gives NaN...
What should we do?
One way of doing it would be to add a || 0 for the autofix, when it is a variable/object. Like that, foo = bar | 0 would become foo = Math.trunc(bar || 0). But this does not resolve the case where the variable contains a string for example (because 'a' | 0 gives 0)

@sindresorhus
Copy link
Owner

The rule name could be improved though:

  • no-bitwise-number-truncation
  • prefer-math-trunc

@papb
Copy link

papb commented Oct 4, 2020

I like prefer-math-trunc 👍

@noftaly noftaly changed the title Add no-bitwise-trunc rule Add prefer-math-trunc rule Oct 5, 2020
@noftaly
Copy link
Contributor Author

noftaly commented Oct 5, 2020

I renamed the rule and adressed all reviews :)
The CI seems to be failing, but because of another rule, not prefer-math-trunc. npm run test is successful on my laptop, which is up to date with this PR...
nevermind, i just did not sort the rule alphabetically after renaming it.

@noftaly noftaly requested review from fisker and sindresorhus October 5, 2020 13:06
Copy link
Collaborator

@fisker fisker left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Please add some cases that have comments inside or around. Also some parenthesized cases.

@sindresorhus sindresorhus merged commit 5ee2432 into sindresorhus:master Oct 6, 2020
@sindresorhus
Copy link
Owner

Nice work, @noftaly 👍🏻

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

Successfully merging this pull request may close these issues.

Rule proposal: no-bitwise-trunc
4 participants