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(biome_analyze): add RuleAction::new #2820

Merged

Conversation

dyc3
Copy link
Contributor

@dyc3 dyc3 commented May 12, 2024

Summary

For #2799, we want to phase out the manual construction of the RuleAction struct to make the applicability field private.

This is intensionally small to make it easy to review before I do a huge codemod to actually phase out the manual construction.

related to: #2799

Test Plan

It compiles.

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

@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.

Forgot to mention that, but this should be impl Into<Applicability>, so inside the rule we can pass ctx.metadata().fix_kind

Copy link

codspeed-hq bot commented May 12, 2024

CodSpeed Performance Report

Merging #2820 will not alter performance

Comparing dyc3:05-12-refactor_biome_analyze_add_ruleaction_new_ (40b9736) with main (b33c9df)

Summary

✅ 99 untouched benchmarks

@dyc3 dyc3 force-pushed the 05-12-refactor_biome_analyze_add_ruleaction_new_ branch from f68d593 to c673e9d Compare May 12, 2024 12:33
@dyc3 dyc3 requested a review from ematipico May 13, 2024 11:45
@dyc3 dyc3 force-pushed the 05-12-refactor_biome_analyze_add_ruleaction_new_ branch from c673e9d to fc0dd8a Compare May 13, 2024 11:50
pub fn new(
category: ActionCategory,
applicability: impl Into<Applicability>,
message: MarkupBuf,
Copy link
Member

Choose a reason for hiding this comment

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

I think this can be changed into message: Markup, so we can move the .to_owned() inside this function, the caller won't need to do it anymore

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Based on this SO answer I don't think impl ToOwned is the right way to go, at least for now. I think that would require significant changes to how Markup and MarkupBuf are implemented.

Copy link
Member

Choose a reason for hiding this comment

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

Actually, we need impl biome_console::fmt::Display.

Then inside the new function, we call markup!().to_owned

@dyc3 dyc3 force-pushed the 05-12-refactor_biome_analyze_add_ruleaction_new_ branch from fc0dd8a to 679fea3 Compare May 13, 2024 12:22
@dyc3 dyc3 force-pushed the 05-12-refactor_biome_analyze_add_ruleaction_new_ branch 2 times, most recently from 73c6cb1 to 8b0ee6a Compare May 13, 2024 12:54
@dyc3 dyc3 requested a review from ematipico May 14, 2024 15:20
@dyc3 dyc3 force-pushed the 05-12-refactor_biome_analyze_add_ruleaction_new_ branch 2 times, most recently from 3fe941a to f42b6ce Compare May 14, 2024 16:52
@dyc3 dyc3 force-pushed the 05-12-refactor_biome_analyze_add_ruleaction_new_ branch from f42b6ce to 40b9736 Compare May 15, 2024 14:00
Copy link
Member

@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.

That's awesome!

@ematipico ematipico merged commit c180aac into biomejs:main May 15, 2024
11 checks passed
@dyc3 dyc3 deleted the 05-12-refactor_biome_analyze_add_ruleaction_new_ branch May 15, 2024 14:22
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.

None yet

2 participants