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.
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.
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.
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.
I added a couple more "invalid" examples.
not-an-aardvark
force-pushed
the
issue6698
branch
from
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. |
not-an-aardvark
force-pushed
the
issue6698
branch
from
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.
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.
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! |
not-an-aardvark
force-pushed
the
issue6698
branch
from
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! |
eslint-deprecated
bot
added
the
archived due to age
What issue does this pull request address?
#6698
What changes did you make? (Give an overview)
This PR adds a
requireStringLiteralsoption 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.