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

feat(lint/noUselessElse): add rule #331

Merged
merged 3 commits into from
Sep 21, 2023
Merged

Conversation

Conaclos
Copy link
Member

@Conaclos Conaclos commented Sep 18, 2023

Summary

Fixes #247.

I am not 100% sold on the rule name. I think that noUselessElse doesn't communicate the actual intent of the rule. Unfortunately I didn't find a better name yet. Clippy uses no_redundant_else.

Trivia handling is again a pain. I finally gave up and keep the suggested code mis-indented.
We should certainly implement a kind of "mini formatter" for the code fixes.
An alternative is to remove the code fix of the rule...

Test Plan

Tests included

@Conaclos Conaclos temporarily deployed to Website deployment September 18, 2023 15:38 — with GitHub Actions Inactive
@github-actions github-actions bot added A-Project Area: project A-Linter Area: linter A-Website Area: website L-JavaScript Language: JavaScript and super languages A-Diagnostic Area: diagnostocis labels Sep 18, 2023
@Conaclos Conaclos force-pushed the conaclos/lint/noUselessElse branch from 69c13ba to a07cae0 Compare September 19, 2023 22:12
@Conaclos Conaclos temporarily deployed to Website deployment September 19, 2023 22:12 — with GitHub Actions Inactive
@github-actions github-actions bot added the A-Core Area: core label Sep 19, 2023
@Conaclos Conaclos force-pushed the conaclos/lint/noUselessElse branch from a07cae0 to a6e02a7 Compare September 19, 2023 22:13
@Conaclos Conaclos temporarily deployed to Website deployment September 19, 2023 22:14 — with GitHub Actions Inactive
@Conaclos Conaclos marked this pull request as ready for review September 19, 2023 22:17
@Conaclos Conaclos requested a review from a team September 19, 2023 22:17
@github-actions
Copy link
Contributor

Parser conformance results on

js/262

Test result main count This PR count Difference
Total 48863 48863 0
Passed 47808 47808 0
Failed 1055 1055 0
Panics 0 0 0
Coverage 97.84% 97.84% 0.00%

jsx/babel

Test result main count This PR count Difference
Total 40 40 0
Passed 37 37 0
Failed 3 3 0
Panics 0 0 0
Coverage 92.50% 92.50% 0.00%

symbols/microsoft

Test result main count This PR count Difference
Total 6212 6212 0
Passed 1764 1764 0
Failed 4448 4448 0
Panics 0 0 0
Coverage 28.40% 28.40% 0.00%

ts/babel

Test result main count This PR count Difference
Total 639 639 0
Passed 571 571 0
Failed 68 68 0
Panics 0 0 0
Coverage 89.36% 89.36% 0.00%

ts/microsoft

Test result main count This PR count Difference
Total 17224 17224 0
Passed 13123 13123 0
Failed 4101 4101 0
Panics 0 0 0
Coverage 76.19% 76.19% 0.00%

@Conaclos Conaclos force-pushed the conaclos/lint/noUselessElse branch from a6e02a7 to 7809e79 Compare September 20, 2023 11:48
@Conaclos Conaclos temporarily deployed to Website deployment September 20, 2023 11:48 — with GitHub Actions Inactive
@Conaclos Conaclos force-pushed the conaclos/lint/noUselessElse branch from 7809e79 to e374b3a Compare September 20, 2023 11:51
@Conaclos Conaclos temporarily deployed to Website deployment September 20, 2023 11:51 — with GitHub Actions Inactive
@ematipico
Copy link
Member

Unfortunately I didn't find a better name yet. Clippy uses no_redundant_else.

I don't mind clippy's name :)

@unvalley
Copy link
Member

unvalley commented Sep 20, 2023

imho, I like noRedundantElse.
And, we have already

  • noRedundantAlt
  • noRedundantRole
  • noRedundantUseStrict

PS: oh, we also have

  • noUselessLabel
  • noUselessFragments
  • noUselessCatch
  • noUselessRename
  • noUselessThisAlias
  • noUselessSwitchCase

maybe a chance to create naming rule for lints

@Conaclos
Copy link
Member Author

Conaclos commented Sep 20, 2023

maybe a chance to create naming rule for lints

I personally prefer Useless to Redundant. Not sure if we should deprecate a rule just because of its name?
One rule I would like to rename is noMultipleSpacesInRegularExpression (other regex-specific rules use Regex instead of RegularExpression).

@ematipico @unvalley
Do you prefer noRedundnatElse over noUselessElse? Some reasons?

Also, Should we recommend the rule?

@unvalley
Copy link
Member

unvalley commented Sep 21, 2023

Not sure if we should deprecate a rule just because of its name?

I think we shouldn't. Editing biome.json by name change is kinda bothering.

Do you prefer noRedundnatElse over noUselessElse? Some reasons?

As for me it's just preference. No strong reason.

Also, should we recommend the rule?

Nice! This rule helps readability and reducing waste code.

@ematipico
Copy link
Member

ematipico commented Sep 21, 2023

I don't have strong opinions nor preferences. I like both.

I definitely agree we should settle on one single naming style, and updating the internal convention, so new rules will follow it .

For renaming existing rules, that's breaking material, so we can worry about it later in the future.

@Conaclos
Copy link
Member Author

I definitely agree we should settle on one single naming style, and updating the internal convention, so new rules will follow it.

Ok, I will keep Useless because it is the most used form.

For renaming existing rules, that's breaking material, so we can worry about it later in the future.

Yes. However, I think earlier the breaking change is, better is.

@Conaclos Conaclos force-pushed the conaclos/lint/noUselessElse branch from e374b3a to de77c0e Compare September 21, 2023 13:14
@Conaclos Conaclos temporarily deployed to Website deployment September 21, 2023 13:14 — with GitHub Actions Inactive
@github-actions github-actions bot added the A-Changelog Area: changelog label Sep 21, 2023
@Conaclos Conaclos force-pushed the conaclos/lint/noUselessElse branch from de77c0e to 544da95 Compare September 21, 2023 13:15
@Conaclos Conaclos temporarily deployed to Website deployment September 21, 2023 13:15 — with GitHub Actions Inactive
@Conaclos Conaclos temporarily deployed to Website deployment September 21, 2023 14:07 — with GitHub Actions Inactive
@Conaclos Conaclos temporarily deployed to Website deployment September 21, 2023 14:10 — with GitHub Actions Inactive
@Conaclos Conaclos merged commit 4afe0af into main Sep 21, 2023
18 checks passed
@Conaclos Conaclos deleted the conaclos/lint/noUselessElse branch September 21, 2023 14:37
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
A-Changelog Area: changelog A-Core Area: core A-Diagnostic Area: diagnostocis A-Linter Area: linter A-Project Area: project A-Website Area: website L-JavaScript Language: JavaScript and super languages
Projects
None yet
Development

Successfully merging this pull request may close these issues.

📎 Implement lint/noUselessElse - eslint/no-else-return
3 participants