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

feat(rome_js_analyze): noUnusedLabels #4096

Merged
merged 1 commit into from
Jan 4, 2023
Merged

feat(rome_js_analyze): noUnusedLabels #4096

merged 1 commit into from
Jan 4, 2023

Conversation

Conaclos
Copy link
Contributor

Summary

Closes #3991

I implemented the rule with the suggestion of @ematipico: I am using a custom visitor.

I have some doubts about the quick fix action: Similarity to noUnusedVariables, I suggest prepending with an underscore.
Thus, the rule ignores unused labels that start with an underscore.

Another possibility could be to suggest the removal of the label.

Test Plan

ESLint's unit tests included.

Documentation

  • The PR requires documentation
  • I will create a new PR to update the documentation

@netlify
Copy link

netlify bot commented Dec 22, 2022

Deploy Preview for docs-rometools ready!

Built without sensitive environment variables

Name Link
🔨 Latest commit b31450f
🔍 Latest deploy log https://app.netlify.com/sites/docs-rometools/deploys/63b46b50314d620009226710
😎 Deploy Preview https://deploy-preview-4096--docs-rometools.netlify.app
📱 Preview on mobile
Toggle QR Code...

QR Code

Use your smartphone camera to open QR code link.

To edit notification comments on pull requests, go to your Netlify site settings.

Copy link
Contributor

@ematipico ematipico left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The code looks good; I suggest the following changes:

  • internally, we spoke about the "prepend underscores to variable names" and we came to the conclusion is not a good practice from our side (we're forcing a habit on users). Hence we should follow the same here and avoid the _;
  • As for the code action, I think it's best to follow ESLint's lead and remove the label; the code fix should be safe;

@Conaclos
Copy link
Contributor Author

I made the change you suggested. However, I do not know how to easily transfer the trailing comment of token : to the next token since the next token is part of a JsStatement which replaces JsLabelStatement. The node is thus moved: this disables any mutation in it.

I changed HashMap to FxHashMap for perf reason.

I also fixed an issue: I forgot to take control flow roots into account.
I added unit tests to avoid regression on this.

@ematipico
Copy link
Contributor

I made the change you suggested. However, I do not know how to easily transfer the trailing comment of token : to the next token since the next token is part of a JsStatement which replaces JsLabelStatement. The node is thus moved: this disables any mutation in it.

In that regard, we could also decide to not keep the trivia and remove it. I think we already have some code action that do so. Although, in that case, the code action should be suggested.

@Conaclos
Copy link
Contributor Author

In that regard, we could also decide to not keep the trivia and remove it. I think we already have some code action that do so. Although, in that case, the code action should be suggested.

Ok. The PR should thus be fine :)

@Conaclos Conaclos requested review from ematipico and removed request for xunilrj and leops December 23, 2022 18:15
@ematipico ematipico merged commit b6e7a39 into rome:main Jan 4, 2023
@Conaclos Conaclos deleted the no_unused_labels branch March 7, 2023 16:29
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

noUnusedLabel, no-unused-labels
2 participants