Update: prefer-numeric-literals warns Number.parseInt (fixes #8913) #8929
Conversation
platinumazure
added
accepted
|
@platinumazure, thanks for your PR! By analyzing the history of the files in this pull request, we identified @not-an-aardvark and @annie to be potential reviewers. |
|
LGTM |
lib/rules/prefer-numeric-literals.js
Outdated
| * false otherwise. | ||
| */ | ||
| function isParseInt(calleeNode) { | ||
| if (calleeNode) { |
There was a problem hiding this comment.
Is this null check necessary? I think CallExpressions always have callee nodes.
There was a problem hiding this comment.
It's probably not strictly necessary, do you believe it needs to be removed?
There was a problem hiding this comment.
I personally don't like having unused checks "just in case". If a CallExpression doesn't have a callee for whatever reason, some invariant has been violated and the rule is likely to do the wrong thing anyway, since we don't know what the "right thing" would be. This also makes it much harder to detect bugs in other parts of the code (in this case, the parser) because the rule just fails silently rather than throwing if it encounters something unexpected.
There was a problem hiding this comment.
Okay, I can remove it. Maybe you should switch your review to Request Changes so nobody accidentally merges this in?
|
@gyandeeps Are you talking about things like |
|
@platinumazure those are the things i was talking about. Thanks for checking. |
|
LGTM |
| * @returns {boolean} True if the callee is `parseInt` or `Number.parseInt`, | ||
| * false otherwise. | ||
| */ | ||
| function isParseInt(calleeNode) { |
There was a problem hiding this comment.
Could probably be done a lot simple with selectors, but this is fine.
|
@ilyavolodin You're right, although that wouldn't solve the shadowing issues called out
by @gyandeeps. So that's why I'm not bothering with any deeper refactoring
at this point. Thanks for reviewing!
…On Jul 15, 2017 4:20 PM, "Ilya Volodin" ***@***.***> wrote:
***@***.**** approved this pull request.
------------------------------
In lib/rules/prefer-numeric-literals.js
<#8929 (comment)>:
> @@ -6,13 +6,40 @@
"use strict";
//------------------------------------------------------------------------------
+// Helpers
+//------------------------------------------------------------------------------
+
+/**
+ * Checks to see if a CallExpression's callee node is `parseInt` or
+ * `Number.parseInt`.
+ * @param {ASTNode} calleeNode The callee node to evaluate.
+ * @returns {boolean} True if the callee is `parseInt` or `Number.parseInt`,
+ * false otherwise.
+ */
+function isParseInt(calleeNode) {
Could probably be done a lot simple with selectors, but this is fine.
—
You are receiving this because you were mentioned.
Reply to this email directly, view it on GitHub
<#8929 (review)>,
or mute the thread
<https://github.com/notifications/unsubscribe-auth/AARWekQhkydl9gcBKFkPyHGBDSaENQf-ks5sOS0cgaJpZM4OVJNo>
.
|
eslint-deprecated
bot
added
the
archived due to age
What is the purpose of this pull request? (put an "X" next to item)
[ ] Documentation update
[ ] Bug fix (template)
[ ] New rule (template)
[x] Changes an existing rule (template)
[ ] Add autofixing to a rule
[ ] Add a CLI option
[ ] Add something to the core
[ ] Other, please explain:
See #8913.
What changes did you make? (Give an overview)
The
prefer-numeric-literalsrule now warns onNumber.parseIntinvocations as well asparseIntinvocations.Is there anything you'd like reviewers to focus on?
Nothing in particular.