-
-
Notifications
You must be signed in to change notification settings - Fork 4.4k
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
Update: requireStringLiterals
option for valid-typeof
(fixes #6698)
#6923
Conversation
@not-an-aardvark, thanks for your PR! By analyzing the annotation information on this pull request, we identified @vitorbal, @pmcelhaney and @iancmyers to be potential reviewers |
LGTM |
```js | ||
typeof foo === "undefined" | ||
typeof bar == "object" | ||
typeof baz === "string" |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Wondering if we want to mention something like typeof thing = "foo"
? To clarify that we're not checking if it's a valid string literal.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Never mind, I guess we are checking that. In that case I would love the docs to mention this and the example I proposed should be listed in incorrect code.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
There's typeof baz === "strnig"
, which certainly signifies the intent (to catch typos), but it's easy to overlook. I'm +1 for adding typeof baz = "foo"
or typeof baz = "someInvalidType"
as another example.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I added a couple more "invalid" examples.
a01a003
to
d4d8bf0
Compare
LGTM |
Left some comments on a previous commit. I'm not sure why they're not showing up in the conversation view. |
@pmcelhaney I think it's because you left the comments on the commit itself rather than this PR. |
d4d8bf0
to
5c9b057
Compare
LGTM |
context.report(sibling, "Invalid typeof comparison value."); | ||
} | ||
} else if (context.options[0] && context.options[0].requireStringLiterals) { | ||
context.report(sibling, "Invalid typeof comparison to a non-string literal."); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
To avoid the double negative, what about saying "Typeof comparisons should be to string literals."? (I'm not thrilled with the wording of that either, so I'd be open to better phrasings.)
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I couldn't think of anything better, so I went with "Typeof comparisons should be to string literals."
Nice work on this, @not-an-aardvark! I'd like to tweak the message a bit to avoid a double negative, but everything else looks good! |
5c9b057
to
847862d
Compare
LGTM |
LGTM, leaving open for another day since the issue wasn't accepted until yesterday. |
LGTM as well. |
Thanks @not-an-aardvark! |
What issue does this pull request address?
#6698
What changes did you make? (Give an overview)
This PR adds a
requireStringLiterals
option tovalid-typeof
, to enforce comparisons to string literals.Is there anything you'd like reviewers to focus on? Not really
I noticed that #6698 hasn't been marked as "accepted" yet, but it seemed like there was enough support that it was worth making this PR. I realize that I'll probably have to wait for that issue to get accepted before this PR gets merged, though.