test: More test cases for no-case-declarations

[ElianCordoba]

Prerequisites checklist

• I have read the contributing guidelines.

What is the purpose of this pull request? (put an "X" next to an item)

[ ] Documentation update
[ ] Bug fix (template)
[ ] New rule (template)
[ ] Changes an existing rule (template)
[ ] Add autofix to a rule
[ ] Add a CLI option
[ ] Add something to the core
[x] Other, please explain:
A (minor) perf improvement

What changes did you make? (Give an overview)

Improved heuristic for the rule, if there are no consequents or if the case block contains a block statement, then we can determine that the rule does not need to be executed.

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

I benchmarked this with the TIMING env variable but sadly I didn't see any improvements, leaving this up to the maintainers if you want to merge it or not. I didn't know how to bench this in a larger codebase, maybe with npm link?

[linux-foundation-easycla]

The committers listed above are authorized under a signed CLA.

• ✅ login: ElianCordoba / name: Elian Cordoba (6105a8d, 909d3e7)

[fasttime]

Thanks for your interest in contributing to ESLint @ElianCordoba.

I think this change does not preserve the current behavior. For example, a code like this should report an error:

/* eslint no-case-declarations: 2 */

switch (a) { case 1: {} function f() {} break; }

DEMO

But it no longer does after this change. What kind of code are you expecting to be impacted by this optimization?

[ElianCordoba]

Ah you are right, the current logic won't catch that example. So, this is the original case I was trying improve:

If the case clause is empty or, if it has the brackes (having only one consequents being a Block statement), then we know that we can skip the check.

I updated my branch with a change that covers the example that @fasttime shared, I will also add it as a test case.

[eslint-github-bot]

Hi @ElianCordoba!, thanks for the Pull Request

The first commit message isn't properly formatted. We ask that you update the message to match this format, as we use it to generate changelogs and automate releases.

• The commit message tag wasn't recognized. Did you mean "docs", "fix", or "feat"?
• There should be a space following the initial tag and colon, for example 'feat: Message'.
• The first letter of the tag should be in lowercase

To Fix: You can fix this problem by running git commit --amend, editing your commit message, and then running git push -f to update this pull request.

Read more about contributing to ESLint here

[BYK]

I appreciate the new test cases that said the scenario this is optimizing for does not seem worth optimizing to me:

1. If node.consequent.length is 0, then the for loop will not be executed at all, so there should be no visible improvement for that case
2. If node.consequent.length is 1, then the for loop will only be run once which does almost exactly the same thing you are doing in the if statement -- take the first element, compare one of its attributes with a string.

So my inclination is to reject this change as it does add more code and logic but the return on investment on that seems 0 or very close to 0.

What do you think?

[BYK]

Better to check "the only element" part earlier

Suggested change

	node.consequent[0].type === "BlockStatement" && node.consequent.length === 1 // Clause has {} around child expressions
	node.consequent.length === 1 && node.consequent[0].type === "BlockStatement" // Clause has {} around child expressions

[mdjermanovic]

1. If node.consequent.length is 0, then the for loop will not be executed at all, so there should be no visible improvement for that case
2. If node.consequent.length is 1, then the for loop will only be run once which does almost exactly the same thing you are doing in the if statement -- take the first element, compare one of its attributes with a string.

So my inclination is to reject this change as it does add more code and logic but the return on investment on that seems 0 or very close to 0.

I agree with @BYK, it doesn't seem that the added code would really improve the performance.

Test cases look useful. @ElianCordoba if you want, you can undo the code changes and we'll merge the tests.

[netlify]

✅ Deploy Preview for docs-eslint canceled.

Name	Link
🔨 Latest commit	909d3e7
🔍 Latest deploy log	https://app.netlify.com/sites/docs-eslint/deploys/649f248c3e888400082e0ea2

[ElianCordoba]

Hi @BYK and @mdjermanovic!

I agree, it's not worth the extra complexity, I took the suggestion and undid the logic changes and left the new tests.

Thanks for the guidance!

[fasttime]

LGTM. I just left some notes.

[ElianCordoba]

Fixed @fasttime!

[mdjermanovic]

LGTM, thanks!