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

refactor(codemod/biome_js_analyze): derive Applicability for all lint rule actions #2889

Conversation

dyc3
Copy link
Contributor

@dyc3 dyc3 commented May 16, 2024

Summary

Following up on the previous PR, #2885, this changes all the JS rules to derive their applicability from the FixKind supplied in the metadata. Should completely resolve #2799.

closes #2799
closes #2797

Test Plan

Should compile and pass tests.

@github-actions github-actions bot added A-Linter Area: linter L-JavaScript Language: JavaScript and super languages labels May 16, 2024
Copy link

codspeed-hq bot commented May 16, 2024

CodSpeed Performance Report

Merging #2889 will not alter performance

Comparing dyc3:05-16-refactor_codemod_biome_js_analyze_derive_applicability_for_all_lint_rule_actions (cd99dc8) with main (a678c68)

Summary

✅ 99 untouched benchmarks

@dyc3 dyc3 marked this pull request as ready for review May 16, 2024 15:25
@dyc3 dyc3 force-pushed the 05-16-refactor_codemod_biome_js_analyze_derive_applicability_for_all_lint_rule_actions branch from d895aff to 8f19c6d Compare May 16, 2024 15:54
@dyc3
Copy link
Contributor Author

dyc3 commented May 17, 2024

To get the tests to pass, I had to change the fix_kind for these rules:

  • useRegexLiterals
  • useNumberNamespace
  • noUnusedLabels
  • noUselessLoneBlockStatements

All of these appeared to be emitting safe fixes before, so I've updated the fix_kind accordingly.

@dyc3 dyc3 force-pushed the 05-16-refactor_codemod_biome_js_analyze_derive_applicability_for_all_lint_rule_actions branch from 8f19c6d to 9738124 Compare May 17, 2024 20:24
@Conaclos
Copy link
Member

Conaclos commented May 17, 2024

Should we implement Into<Applicability> for RuleMetadata? This could allow removing to_applicability() and just pass ctx.metadata(). If we keep to_applicability(), I suggest a distinct name: applicability().

@dyc3
Copy link
Contributor Author

dyc3 commented May 17, 2024

I don't think so. You're right in that it would reduce some of the boilerplate, but IMO it's inconsequential. To me, the From and Into traits represent an equivalence relationship between 2 types, and implementing Into<Applicability> violates that because it loses the majority of the information.

I do agree with the name change though.

@Conaclos
Copy link
Member

Fair enough. Let's change the name and merge it :)

@ematipico
Copy link
Member

Fair enough. Let's change the name and merge it :)

I suggested to_applicability because it follows rust naming conventions

@Conaclos
Copy link
Member

Conaclos commented May 18, 2024

I suggested to_applicability because it follows rust naming conventions

I suggested implementing into because the to_ misled me.
But this is not a conversion. So as_/to_/into_ doesn't apply here?
Moreover, according to the resource you point to, to_ indicates a costly operation. This is not the case here?

@ematipico
Copy link
Member

ematipico commented May 18, 2024

Yeah it makes sense, now that I read more of the docs.

applicability() makes more sense

@dyc3 dyc3 force-pushed the 05-16-refactor_codemod_biome_js_analyze_derive_applicability_for_all_lint_rule_actions branch from 9738124 to cd99dc8 Compare May 18, 2024 11:35
@Conaclos Conaclos merged commit 2226bae into biomejs:main May 18, 2024
11 checks passed
@dyc3 dyc3 deleted the 05-16-refactor_codemod_biome_js_analyze_derive_applicability_for_all_lint_rule_actions branch May 18, 2024 12:07
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
A-Linter Area: linter L-JavaScript Language: JavaScript and super languages
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Compute fix kind from metatadata ☂️ Analyzer internal refactors
3 participants