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

Update: allow continue instead of if wrap in guard-for-in (fixes #7567) #9796

Merged
merged 1 commit into from Jan 20, 2018

Conversation

@michaelficarra
Copy link
Member

michaelficarra commented Jan 2, 2018

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:

What changes did you make? (Give an overview)

Is there anything you'd like reviewers to focus on?

@ljharb

This comment has been minimized.

Copy link
Contributor

ljharb commented Jan 2, 2018

Could this be behind an option? I want to preserve the current behavior (disallowing continue statements).

@michaelficarra

This comment has been minimized.

Copy link
Member Author

michaelficarra commented Jan 2, 2018

@ljharb I don't think that'd be appropriate here. This rule is not stylistic. It intends to protect you from a potential hazard, and either pattern is safe.

@platinumazure

This comment has been minimized.

Copy link
Member

platinumazure commented Jan 2, 2018

I agree this should probably be behind an option. , and it seems if we want to allow continue, we should also allow break in the same way.

Let's discuss in the issue.

@ljharb

This comment has been minimized.

Copy link
Contributor

ljharb commented Jan 2, 2018

Since the rule isn't checking what the guard is doing - ie, it's not enforcing a hasOwnProperty check - it's not actually protecting against a hazard. I've seen plenty of people accidentally bypass this rule by checking something besides hasOwnProperty.

@platinumazure

This comment has been minimized.

Copy link
Member

platinumazure commented Jan 2, 2018

@michaelficarra

This comment has been minimized.

Copy link
Member Author

michaelficarra commented Jan 2, 2018

@platinumazure break/return/throw wouldn't be used to avoid iterating prototype properties (while still iterating own properties) like continue is.

@platinumazure

This comment has been minimized.

Copy link
Member

platinumazure commented Jan 4, 2018

@ljharb Could you use no-restricted-syntax to catch the style that you don't want, while allowing this change to occur without a new option? Just wondering... We could certainly add a new option later if we needed to (i.e., if enough people asked for it).

@ljharb

This comment has been minimized.

Copy link
Contributor

ljharb commented Jan 5, 2018

@platinumazure if we want to ban continue, then yes, but if we want to allow continue (just not in this style) then we couldn't. I think it should go behind an option.

@michaelficarra

This comment has been minimized.

Copy link
Member Author

michaelficarra commented Jan 5, 2018

I still think that an option here is inappropriate for the reasons I mentioned above. This rule is listed in the Best Practices section, not the Stylistic Issues section. Using it to enforce a particular style is an abuse of this rule. And without an option, it still always protects you from the possible hazard.

@ilyavolodin

This comment has been minimized.

Copy link
Member

ilyavolodin commented Jan 5, 2018

I do agree with @michaelficarra here. This rule is in a different category then things like no-continue. It's not really meant for styleguide use (as far as I understand that's the problem @ljharb has with adding it in without an option). The style rule should be the one that allows/disallows continue inside specific nodes. I know we don't have available right now, but it can be created as a plugin.

@ljharb

This comment has been minimized.

Copy link
Contributor

ljharb commented Jan 5, 2018

"styleguide use" covers all categories of rules; my issue with adding it without an option is that i want the entire body of the for loop to be surrounded by the same single condition, and i want the extra nesting, so that the existence of the condition is clear.

It is not a "best practice" to early-continue a for loop under a guard (nor is it a bad practice). In fact, it's simply @michaelficarra's preferred style here.

In terms of protecting against a hazard, this rule does not actually protect against the hazard - it simply requires any condition.

This rule is aimed at preventing unexpected behavior that could arise from using a for in loop without filtering the results in the loop.

The unexpected behavior is when you encounter an inherited property; filtering the results isn't a generic solution to that problem.

@platinumazure

This comment has been minimized.

Copy link
Member

platinumazure commented Jan 5, 2018

I don't see why we couldn't add an option here-- after all, @michaelficarra could certainly use the option in his own setup.

Can someone convince me why adding an option would be a bad idea?

@not-an-aardvark

This comment has been minimized.

Copy link
Member

not-an-aardvark commented Jan 5, 2018

It makes the rule slightly more complicated for users to configure, and also slightly more complicated for us to maintain. Additionally, the option seems to be tangential to the main purpose of the rule. In this case I don't think it's worthwhile.

@michaelficarra

This comment has been minimized.

Copy link
Member Author

michaelficarra commented Jan 6, 2018

@ljharb I'd love for the rule to be stricter about the condition of the guard, but I think that change is inappropriate for this PR. Also, "best practice" (probably not the best term) is defined on that page as

better ways of doing things to help you avoid problems

and I believe that both approaches allowed by this rule accomplish that goal.

edit: To emphasise my point, if there was a third way to reliably and generally protect against the enumerable prototype properties hazard, I would want this rule to allow that pattern as well, even if I did not personally plan to use that pattern.

Copy link
Member

not-an-aardvark left a comment

Looks good to me, thanks!

Copy link
Member

platinumazure left a comment

LGTM, thanks for contributing!

@platinumazure platinumazure merged commit e26a25f into master Jan 20, 2018
5 checks passed
5 checks passed
commit-message Commit message follows guidelines
Details
continuous-integration/appveyor/pr AppVeyor build succeeded
Details
continuous-integration/travis-ci/pr The Travis CI build passed
Details
licence/cla Contributor License Agreement is signed.
Details
release-monitor No patch release is pending
Details
@not-an-aardvark not-an-aardvark deleted the GH-7567 branch Jan 20, 2018
@renovate renovate bot mentioned this pull request Feb 14, 2018
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Projects
None yet
Linked issues

Successfully merging this pull request may close these issues.

None yet

5 participants
You can’t perform that action at this time.