Skip to content
This repository has been archived by the owner on Aug 31, 2023. It is now read-only.

🐛 Linter js/noNegationElse becomes unreadable with else if blocks #2999

Closed
1 task done
ZachHaber opened this issue Aug 3, 2022 · 0 comments · Fixed by #3001
Closed
1 task done

🐛 Linter js/noNegationElse becomes unreadable with else if blocks #2999

ZachHaber opened this issue Aug 3, 2022 · 0 comments · Fixed by #3001
Labels
A-Linter Area: linter L-JavaScript Langauge: JavaScript S-Bug: confirmed Status: report has been confirmed as a valid bug
Milestone

Comments

@ZachHaber
Copy link

ZachHaber commented Aug 3, 2022

Environment information

WIN 10 x64
Rome Installation via VSCode Extension

What happened?

When applying the "fix" for noNegationElse, the code becomes more unreadable and takes several distinct "fix" actions to fix it.

if (!/^NCT/.test(input)) {
  messages.push("NCT Number must start with NCT");
} else if (!/^NCT\d{8}$/.test(input)) {
  messages.push("NCT Number must have exactly 8 digits after NCT");
}

Becomes

if (/^NCT/.test(input)) if (!/^NCT\d{8}$/.test(input)) {
  messages.push("NCT Number must have exactly 8 digits after NCT");
} else {
  messages.push("NCT Number must start with NCT");
}

Which prompts for wrapping it in a block statement (which is definitely needed to be readable), and then prompts for another noNegationElse fix to finally a very nested mess:

if (/^NCT/.test(input)) {
  if (/^NCT\d{8}$/.test(input)) {
    messages.push("NCT Number must start with NCT");
  } else {
    messages.push("NCT Number must have exactly 8 digits after NCT");
  }
}

This set of chaining fixes will continue on for every not condition in a chain, so if you have more conditions it will end up becoming more nested

Expected result

When working with chains of if/else if values, I believe that this linter rule should ignore them given how much harder it is to read with only 1 else if. Potentially a fix for this issue would be to make it so that only if/else pairs without else if blocks values are considered for this lint rule.

With even more sets in the chain, it becomes nearly impossible to read the end result.

For example this set of if/else if/else blocks takes 7 individual refactor fix commands

if (!/^1/.test(input)) {
  messages.push("NCT Number must start with NCT");
} else if (!/2$/.test(input)) {
  messages.push("NCT Number must have exactly 8 digits after NCT");
} else if (!/^3/.test(input)) {
  // DO something here 3
} else if (!/^4/.test(input)) {
  // DO something here 4
} else {
  // Everything is good! Yay!
}

and becomes this mess:

if (/^1/.test(input)) {
  if (/2$/.test(input)) {
    if (/^3/.test(input)) {
      if (/^4/.test(input)) {
        // Everything is good! Yay!
      } else {
        // DO something here 4
      }
    } else {
      // DO something here 3
    }
  } else {
    messages.push("NCT Number must have exactly 8 digits after NCT");
  }
} else {
  messages.push("NCT Number must start with NCT");
}

Code of Conduct

  • I agree to follow Rome's Code of Conduct
@ZachHaber ZachHaber added the S-To triage Status: user report of a possible bug that needs to be triaged label Aug 3, 2022
@ZachHaber ZachHaber changed the title 🐛 Linter js/noNegationElse 🐛 Linter js/noNegationElse becomes unreadable with else if blocks Aug 3, 2022
@ematipico ematipico added S-Bug: confirmed Status: report has been confirmed as a valid bug A-Linter Area: linter L-JavaScript Langauge: JavaScript and removed S-To triage Status: user report of a possible bug that needs to be triaged labels Aug 4, 2022
@ematipico ematipico added this to the 0.9.0 milestone Aug 4, 2022
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
A-Linter Area: linter L-JavaScript Langauge: JavaScript S-Bug: confirmed Status: report has been confirmed as a valid bug
Projects
Status: Done
2 participants