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

allow negated condition in for-in guards in guard-for-in rule #7567

Closed
michaelficarra opened this issue Nov 8, 2016 · 11 comments · Fixed by Urigo/tortilla#62, mono-js/mono-notifications#5, mono-js/mono-push#5 or terrajs/lib-starter#5 · May be fixed by ali8889/emerald-wallet#4
Assignees
Labels
accepted There is consensus among the team that this change meets the criteria for inclusion archived due to age This issue has been archived; please open a new issue for any further discussion enhancement This change enhances an existing feature of ESLint rule Relates to ESLint's core rules

Comments

@michaelficarra
Copy link
Member

michaelficarra commented Nov 8, 2016

I would like guard-for-in to allow the following pattern in addition to what it currently allows.

for (key in foo) {
    if (!{}.hasOwnProperty.call(foo, key)) continue;
    doSomething(key);
}

To be clear, the difference is that this guard has a negated condition with a continue as the consequent. The continue should be allowed as the only statement in a block as well, to be compatible with that brace style preference.

for (key in foo) {
    if (!{}.hasOwnProperty.call(foo, key)) {
        continue;
    }
    doSomething(key);
}

edit:

What rule do you want to change?

guard-for-in

Does this change cause the rule to produce more or fewer warnings?

Fewer.

How will the change be implemented? (New option, new default behavior, etc.)?

New default behaviour.

Please provide some example code that this change will affect:

for (key in foo) {
    if (!{}.hasOwnProperty.call(foo, key)) continue;
    doSomething(key);
}
for (key in foo) {
    if (!{}.hasOwnProperty.call(foo, key)) {
        continue;
    }
    doSomething(key);
}

What does the rule currently do for this code?

Warns.

What will the rule do after it's changed?

Not warn.

@eslintbot eslintbot added the triage An ESLint team member will look at this issue soon label Nov 8, 2016
@platinumazure
Copy link
Member

Maybe this should be refactored to use code path analysis, so that we could flag the for statement if any path is reachable without having gone through a hasOwnProperty check. Of course, it doesn't help that right now we're not even checking what the IfStatement test looks like...

👍 from me.

@platinumazure
Copy link
Member

Oh, @michaelficarra, can you please edit your post to match this template? Thanks!

@michaelficarra
Copy link
Member Author

@platinumazure Done.

@platinumazure platinumazure added enhancement This change enhances an existing feature of ESLint rule Relates to ESLint's core rules evaluating The team will evaluate this issue to decide whether it meets the criteria for inclusion and removed triage An ESLint team member will look at this issue soon labels Nov 9, 2016
@not-an-aardvark
Copy link
Member

@michaelficarra Are you still interested in pushing this proposal forward?

@michaelficarra
Copy link
Member Author

@not-an-aardvark Yes. What does that involve?

@not-an-aardvark
Copy link
Member

It generally involves drumming up support from the team (see here). Right now there are two 👍s from team members on the proposal, so we would need one more for the proposal to be accepted.

@michaelficarra
Copy link
Member Author

@not-an-aardvark Does my own 👍 count?

@not-an-aardvark
Copy link
Member

No, it does not (it requires three 👍s in addition to a champion).

ccing @eslint/eslint-team (sorry about all the pings today) to see if anyone else is in favor of this.

@mysticatea mysticatea added accepted There is consensus among the team that this change meets the criteria for inclusion and removed evaluating The team will evaluate this issue to decide whether it meets the criteria for inclusion labels Jul 11, 2017
@platinumazure
Copy link
Member

We don't have a champion on this, so I'm setting this back to evaluating. If we don't get a champion in a week or two, we should probably close this.

@platinumazure platinumazure added evaluating The team will evaluate this issue to decide whether it meets the criteria for inclusion and removed accepted There is consensus among the team that this change meets the criteria for inclusion labels Dec 31, 2017
@ilyavolodin ilyavolodin added accepted There is consensus among the team that this change meets the criteria for inclusion and removed evaluating The team will evaluate this issue to decide whether it meets the criteria for inclusion labels Jan 4, 2018
@ilyavolodin
Copy link
Member

@platinumazure Since @michaelficarra created this issue, and filed a PR, I assume he is championing it, so setting it back to accepted.

@platinumazure
Copy link
Member

@ilyavolodin @michaelficarra Sorry for the mixup!

Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.