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

New rule proposal: no-dupe-else-if #12469

Closed
mdjermanovic opened this issue Oct 20, 2019 · 7 comments · May be fixed by rsumner868/librealsense2.1#1, rsumner868/librealsense#4, O330oei/node#4 or O330oei/node#11
Closed

Comments

@mdjermanovic
Copy link
Member

@mdjermanovic mdjermanovic commented Oct 20, 2019

Please describe what the rule should do:

Disallows duplicate test conditions in if-else-if chains.

What category of rule is this? (place an "X" next to just one item)

[X] Warns about a potential error (problem)

Provide 2-3 code examples that this rule will warn about:

if (a > 1) {
    foo();
} else if (a > 1) {
    bar();
}

if (a === 1) {
    foo();
} else if (a === 2) {
    bar();
} else if (a === 3) { 
    baz();
} else if (a === 2) {
    quux();
} else if (a === 5) {
    quuux();
}

Why should this rule be included in ESLint (instead of a plugin)?

It's a possible error. A statement with a duplicate condition can never execute (unless it's an unusual use case with side effects).

This rule is similar to the existing no-duplicate-case. The error could be due to a copy & paste and a long chain.

The test nodes would be compared by their tokens.

Are you willing to submit a pull request to implement this rule?

Yes.

@mdjermanovic
Copy link
Member Author

@mdjermanovic mdjermanovic commented Oct 28, 2019

One more 👍 to accept this.

@mysticatea mysticatea added accepted and removed evaluating labels Oct 28, 2019
@mysticatea
Copy link
Member

@mysticatea mysticatea commented Oct 28, 2019

Now accepted 🎉

@mdjermanovic
Copy link
Member Author

@mdjermanovic mdjermanovic commented Oct 28, 2019

I'm working on this :-)

@aladdin-add
Copy link
Member

@aladdin-add aladdin-add commented Nov 12, 2019

just a thought: for consecutive if, whether or not it should check, e.g.

if(foo === 0) {...}
if(foo === 0) {...} // to warn, or not?

I think it's good to cover it, or behide an option as an enhancement.

@mdjermanovic
Copy link
Member Author

@mdjermanovic mdjermanovic commented Nov 12, 2019

just a thought: for consecutive if, whether or not it should check, e.g.

if(foo === 0) {...}
if(foo === 0) {...} // to warn, or not?

I think it's good to cover it, or behide an option as an enhancement.

I think it's more suitable for a separate rule because this isn't necessarily an error (it could be just a bad practice to repeat conditions instead of having all code in a single block, or it might be even a correct code if foo can be assigned a different value in the first block), unlike certainly unreachable code that's being reported by the rule (assuming there are no side effects in conditions).

@kaicataldo
Copy link
Member

@kaicataldo kaicataldo commented Nov 14, 2019

just a thought: for consecutive if, whether or not it should check, e.g.

if(foo === 0) {...}
if(foo === 0) {...} // to warn, or not?

I think it's good to cover it, or behide an option as an enhancement.

Agreed with @mdjermanovic. I don't think this rule should catch this, since this rule shouldn't be disallowing code like the following:

function checkForB(a) { return a.b; }
function checkForC(a) { return a.c; }

const d = {
  c: true
};

let e = checkForB(d);
if (e !== undefined) {/* Do something */}
e = checkForC(d);
if (e !== undefined) {/* Do something */}

or even if we limit it to consecutive statements, potentially:

function checkForB(a) { return a.b; }
function checkForC(a) { return a.c; }

const d = {
  b: true
};

let e = checkForB(d);
if (e !== undefined) {
  if (typeof e !== 'number') {
    e = checkForC(d);
  } 
}
if (e !== undefined) {/* Do something */}
@aladdin-add
Copy link
Member

@aladdin-add aladdin-add commented Nov 17, 2019

emm.. after thinking for a while, I think it's not fit for this rule:

  • its rulename: no-dupe-else-if :)
  • it can catch some mistakes, but also has a few false positives.
@eslint eslint bot locked and limited conversation to collaborators May 18, 2020
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
You can’t perform that action at this time.