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

feat: plugin loader #4234

Merged
merged 4 commits into from
Oct 19, 2024
Merged

feat: plugin loader #4234

merged 4 commits into from
Oct 19, 2024

Conversation

arendjr
Copy link
Contributor

@arendjr arendjr commented Oct 10, 2024

Summary

Continuation of #4071

Note this doesn't integrate the plugins into the analyzer yet, like the previous one did. This just makes sure they load as we want them to.

Test Plan

Tests added. CI should remain green.

@github-actions github-actions bot added A-CLI Area: CLI A-Core Area: core A-Project Area: project A-Linter Area: linter A-Parser Area: parser A-Diagnostic Area: diagnostocis L-Grit Language: GritQL labels Oct 10, 2024
Copy link

codspeed-hq bot commented Oct 10, 2024

CodSpeed Performance Report

Merging #4234 will degrade performances by 6.8%

Comparing arendjr:plugin-loader (4ea3b51) with next (2b9a69f)

Summary

❌ 1 regressions
✅ 100 untouched benchmarks

⚠️ Please fix the performance issues or acknowledge them on CodSpeed.

Benchmarks breakdown

Benchmark next arendjr:plugin-loader Change
react.production.min_3378072959512366797.js[uncached] 2.1 ms 2.3 ms -6.8%

@arendjr arendjr force-pushed the plugin-loader branch 2 times, most recently from 12821d7 to b1543c4 Compare October 10, 2024 19:15
@arendjr arendjr changed the title WIP: Plugin loader feat: plugin loader Oct 10, 2024
@arendjr arendjr marked this pull request as ready for review October 10, 2024 19:17
@arendjr arendjr requested review from a team October 10, 2024 19:17
@ematipico
Copy link
Member

Should we change the destination of the PR to next?

@arendjr arendjr changed the base branch from main to next October 10, 2024 19:20
@github-actions github-actions bot added the A-Changelog Area: changelog label Oct 10, 2024
@arendjr
Copy link
Contributor Author

arendjr commented Oct 10, 2024

I've rebased onto next. I've also included the Grit upgrade, because it wouldn't cleanly apply otherwise.

@arendjr
Copy link
Contributor Author

arendjr commented Oct 11, 2024

Btw, thinking of the whole manifest thing, while I like this approach for future compatibility and the ability to publish plugins, I think I’ll still implement the ability to refer to refer to Grit files directly from the plugins array. It should be easy enough for us to detect and then instantiate a single-rule plugin and it’ll greatly simplify the use case when someone wants to have one or a few rules in their own repository.

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.

I think the PR should be rebased because there are changes that don't belong here

@@ -0,0 +1,32 @@
[package]
Copy link
Member

Choose a reason for hiding this comment

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

It's possible you created the crate manually or with cargo. If so you, you need to update manually knope.toml and add the crate. We have just new-crate that updates the file automatically

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Done

Comment on lines 83 to 85
trait RangeExt {
fn to_text_range(self) -> TextRange;
}
Copy link
Member

Choose a reason for hiding this comment

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

Documentation is missing: the trait and the function.

crates/biome_plugin_loader/src/diagnostics.rs Outdated Show resolved Hide resolved
crates/biome_plugin_loader/src/lib.rs Outdated Show resolved Hide resolved
Comment on lines 5 to 10
plugin ━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━

× Cannot load plugin manifest

Caused by:
version must be 1
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 these snapshots could be improved by providing the input/files provided.

@github-actions github-actions bot removed the A-Changelog Area: changelog label Oct 14, 2024
@arendjr arendjr force-pushed the plugin-loader branch 5 times, most recently from bd7f2e1 to 9a3d4a5 Compare October 14, 2024 20:12
@github-actions github-actions bot added the L-JavaScript Language: JavaScript and super languages label Oct 19, 2024
@arendjr arendjr merged commit dc52b46 into biomejs:next Oct 19, 2024
9 of 12 checks passed
ematipico pushed a commit that referenced this pull request Oct 23, 2024
ematipico pushed a commit that referenced this pull request Oct 23, 2024
ematipico pushed a commit that referenced this pull request Oct 31, 2024
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-Diagnostic Area: diagnostocis A-Linter Area: linter A-Parser Area: parser A-Project Area: project L-Grit Language: GritQL L-JavaScript Language: JavaScript and super languages
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants