Docs: list another related rule in no-undefined #8467
Conversation
|
@ethanresnick, thanks for your PR! By analyzing the history of the files in this pull request, we identified @nzakas, @michaelficarra and @BYK to be potential reviewers. |
|
|
|
LGTM |
|
Hi @ethanresnick, thanks for the PR! Hmm... This is a tough one for me. It's a pretty limited sort of relationship and potentially one should also link I wonder if there should instead be a paragraph somewhere higher in the documentation which says all uses of If others on the team think this is okay as is, I definitely don't want to stand in the way. I just wanted to throw out a thought, that's all. |
|
@platinumazure If it helps as a datapoint, I hadn't realized that |
|
@not-an-aardvark Thanks, makes sense. I'm just worried that the change might be incomplete. var undefined = "true";It won't pick up on reassignments: undefined = "true";So I think we should figure out what we want to recommend for that case and include that in this change. |
|
I agree that it's not a drop-in replacement, but the rules do share some functionality, so I think it's reasonable to call them "related". |
|
@not-an-aardvark I have no problem with including no-shadow-restricted-names as a related rule anymore, I promise. I just want to make sure the reassign case is also covered. |
|
@platinumazure How would you suggest I edit this to cover the reassign case? |
|
@ethanresnick Looks like So if you just add |
|
LGTM |
|
@platinumazure That makes sense. I've updated the PR to reference no-global-assign too. In the process of adding the new paragraph, I tried to clean up the prose a bit too, so things would still flow. |
|
LGTM |
|
LGTM |
|
Looks like you have some markdown linting errors (trailing space); please review and fix at your convenience and let us know if you need help. Thanks! |
|
LGTM |
There was a problem hiding this comment.
Just a few small requested changes about the narrative flow of the documentation. Basically, we can't really say that being undefined-free is "alternative" in this rule because that is the whole point of the rule. Instead, using no-shadow-restricted-names and no-global-assign should be regarded as "alternative" and should be written second (IMO).
Sorry for all the nitpicks. I'm really looking forward to landing this once we can get the flow in the right order!
docs/rules/no-undefined.md
Outdated
| @@ -14,24 +14,15 @@ function doSomething(data) { | |||
| } | |||
| ``` | |||
|
|
|||
| This represents a problem for `undefined` that doesn't exist for `null`, which is a keyword and primitive value that can neither be overwritten nor shadowed. | |||
| Because `undefined` can be overwritten or shadowed, reading `undefined` can give an unexpected value. (This is not the case for `null`, which is a keyword that always produces the same value.) To guard against this, you can use the [no-global-assign](no-global-assign.md) and [no-shadow-restricted-names](no-shadow-restricted-names.md) rules. | |||
There was a problem hiding this comment.
I like the changes you make before "To guard against this"; however, I think it would make more sense to extract the rest to its own paragraph and make it clear that that is an alternative to using this rule to flag all uses of undefined. (And I would put that paragraph below the next one-- see comment below.)
See comment below
docs/rules/no-undefined.md
Outdated
|
|
||
| All uninitialized variables automatically get the value of `undefined`: | ||
| Alternatively, you can avoid all uses of `undefined`, which is what some style guides recommend. Those style guides then recommend: |
There was a problem hiding this comment.
In the context of this rule, it doesn't make sense to say an undefined-free coding style is "alternative". In this rule, that would be the primary use case. So I would lead this paragraph by saying that this rule allows you to avoid all uses of undefined. Then, below the style guide recommendations, I would suggest moving the excerpt I called out above (starting with "To guard against this") and make that paragraph the new "alternative" paragraph.
The alternative to using this rule (for flagging all uses of undefined) is to enable those two rules, which will flag writes to undefined but allow reads.
Hope this makes sense and doesn't sound arbitrary. Let me know if I can clarify anything!
|
LGTM |
|
@platinumazure Your feedback makes perfect sense, and I've updated the PR to address it. Lmk if anything else needs work, or if you want me to squash the commits. |
|
@not-an-aardvark Since your review, a few changes have been made, mostly at my suggestion. Please feel free to re-review if you wish. |
eslint-deprecated
bot
added
the
archived due to age
What is the purpose of this pull request? (put an "X" next to item)
[x] Documentation update
[ ] Bug fix (template)
[ ] New rule (template)
[ ] Changes an existing rule (template)
[ ] Add autofixing to a rule
[ ] Add a CLI option
[ ] Add something to the core
[ ] Other, please explain:
What changes did you make? (Give an overview)
Addded no-shadow-restricted-names as a related rule to no-undefined's documentation, because some people might want to flag all shadowing of undefined (as the former does) and then not use the
no-undefinedrule.Is there anything you'd like reviewers to focus on?