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(project): new crate biome_configuration #2269

Merged
merged 22 commits into from
Apr 4, 2024

Conversation

unvalley
Copy link
Member

@unvalley unvalley commented Apr 2, 2024

Summary

This PR moves biome_service's config-related code to biome_configuration.
But some code that depends biomie_service (e.g. WorkspaceError) is still there.

Closes #1738 #1742

Test Plan

All tests should pass.

@github-actions github-actions bot added A-CLI Area: CLI A-Core Area: core A-Project Area: project A-Formatter Area: formatter A-Tooling Area: internal tools A-LSP Area: language server protocol labels Apr 2, 2024
Copy link

netlify bot commented Apr 2, 2024

Deploy Preview for biomejs ready!

Name Link
🔨 Latest commit c46270e
🔍 Latest deploy log https://app.netlify.com/sites/biomejs/deploys/660e6c2e0fcd370008daf72f
😎 Deploy Preview https://deploy-preview-2269--biomejs.netlify.app
📱 Preview on mobile
Toggle QR Code...

QR Code

Use your smartphone camera to open QR code link.
Lighthouse
Lighthouse
1 paths audited
Performance: 99 (🔴 down 1 from production)
Accessibility: 97 (no change from production)
Best Practices: 100 (no change from production)
SEO: 93 (no change from production)
PWA: -
View the detailed breakdown and full score reports

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

@unvalley unvalley changed the title refactor: new crate biome_configuration refactor(project): new crate biome_configuration Apr 2, 2024
Copy link

codspeed-hq bot commented Apr 2, 2024

CodSpeed Performance Report

Merging #2269 will improve performances by 6.05%

⚠️ No base runs were found

Falling back to comparing unvalley:biome-configuration (c46270e) with main (19d761a)

Summary

⚡ 1 improvements
✅ 92 untouched benchmarks

Benchmarks breakdown

Benchmark main unvalley:biome-configuration Change
db.json[uncached] 84.3 ms 79.5 ms +6.05%

@@ -111,58 +107,6 @@ impl FromStr for FormatterConfiguration {
}
}

pub fn to_format_settings(
Copy link
Member Author

@unvalley unvalley Apr 2, 2024

Choose a reason for hiding this comment

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

These conversion logics (to_*_settings) are moved in crates/biome_service/src/settings.rs

Copy link
Member Author

Choose a reason for hiding this comment

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

The code that depends on the implementation within the biome_service crate has not been moved out of this crate

}

impl FusedIterator for ConfigurationDiagnosticsIter<'_> {}
pub trait PartialConfigurationExt {
Copy link
Member Author

@unvalley unvalley Apr 2, 2024

Choose a reason for hiding this comment

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

This new trait to implement logic that depends on the biome_serivce's implementation.

@unvalley unvalley marked this pull request as ready for review April 2, 2024 18:42
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.

Feel free to merge it once the conflicts are fixed

Copy link
Contributor

@arendjr arendjr left a comment

Choose a reason for hiding this comment

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

🚀

Copy link
Member Author

@unvalley unvalley Apr 4, 2024

Choose a reason for hiding this comment

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

@ematipico @Conaclos (I saw you recently updated it)
I'm unsure about how these snapshot files /biome_configuration/src/snapshots/*.snap (previously at biome_service/src/configuration/snapshots/*.snap) are executed, because I couldn't find the original snapshot files (here it should be some json files). please let me know if you know anything about it.

Copy link
Member

Choose a reason for hiding this comment

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

I just updated this snapshot by running just test. I have no more info...

@ematipico ematipico merged commit 7e0242b into biomejs:main Apr 4, 2024
16 checks passed
@unvalley unvalley deleted the biome-configuration branch April 4, 2024 15:58
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
A-CLI Area: CLI A-Core Area: core A-Formatter Area: formatter A-LSP Area: language server protocol A-Project Area: project A-Tooling Area: internal tools
Projects
None yet
Development

Successfully merging this pull request may close these issues.

📎 Move Biome configuration in its own crate
4 participants