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): remove manual construction of JsRuleAction #2874

Conversation

dyc3
Copy link
Contributor

@dyc3 dyc3 commented May 15, 2024

Summary

This is a follow up to #2820.

Related to: #2799

Test Plan

It compiles, tests pass.

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

codspeed-hq bot commented May 15, 2024

CodSpeed Performance Report

Merging #2874 will not alter performance

Comparing dyc3:05-12-refactor_codemod_biome_js_analyze_remove_manual_construction_of_jsruleaction_ (243c60c) with main (d9e1873)

Summary

✅ 99 untouched benchmarks

@ematipico
Copy link
Member

CI is failing due to clippy error

@dyc3 dyc3 force-pushed the 05-12-refactor_codemod_biome_js_analyze_remove_manual_construction_of_jsruleaction_ branch from 77680aa to 5dbf3e0 Compare May 15, 2024 18:13
@dyc3 dyc3 force-pushed the 05-12-refactor_codemod_biome_js_analyze_remove_manual_construction_of_jsruleaction_ branch from 5dbf3e0 to 243c60c Compare May 15, 2024 18:37
@Conaclos Conaclos merged commit 952527c into biomejs:main May 16, 2024
12 checks passed
@@ -26,7 +26,7 @@ declare_rule! {
///
/// ```ts,expect_diagnostic
/// class SomeClass {
/// message: Array<Array<any>>;
Copy link
Member

Choose a reason for hiding this comment

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

This is probably a result of find and replace that breaks the tests in the website repo:

https://github.com/biomejs/website/actions/runs/9109965581/job/25043948850#step:8:378

@@ -43,7 +43,7 @@ declare_rule! {
///
/// ```ts
/// class SomeClass<T extends any> {
/// message: Array<Array<unknown>>;
Copy link
Member

Choose a reason for hiding this comment

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

This is probably a result of find and replace that breaks the tests in the website repo:

https://github.com/biomejs/website/actions/runs/9109965581/job/25043948850#step:8:378

Copy link
Member

@Sec-ant Sec-ant May 16, 2024

Choose a reason for hiding this comment

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

You forgot to fix this one in 7bcc112 @ematipico

I fixed it in #2883.

@ematipico
Copy link
Member

cc @dyc3

@ematipico
Copy link
Member

Fixed in 7bcc112

@dyc3
Copy link
Contributor Author

dyc3 commented May 16, 2024

Whoops, my bad. It was definitely because I find/replaced that text. Do you have a better way of doing these kinds of large codemods? I'm not aware of any tools that do that

@dyc3 dyc3 deleted the 05-12-refactor_codemod_biome_js_analyze_remove_manual_construction_of_jsruleaction_ branch May 16, 2024 11:32
@Sec-ant
Copy link
Member

Sec-ant commented May 16, 2024

Whoops, my bad. It was definitely because I find/replaced that text. Do you have a better way of doing these kinds of large codemods? I'm not aware of any tools that do that

I've never used it in repos like biome but you can try gritql which is also what biome plans to use to support plugins.

Anyway these kind of errors are supposed to be catched by tests, it's just a limitation of our current setup (main repo + website repo codegen) which makes these kind of errors more difficult to avoid.

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.

4 participants