-
-
Notifications
You must be signed in to change notification settings - Fork 792
feat: add plugins to override config #6117
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
Conversation
4ba588b to
7cf5588
Compare
arendjr
left a comment
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Good stuff! Looks like you found your way well around the important pieces 💪
| result.extend_from_slice(&plugin.analyzer_plugins); | ||
| } | ||
| None => { | ||
| panic!("plugin not loaded: {plugin_path}") |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This looks like something that should be gracefully handled still.
crates/biome_service/src/settings.rs
Outdated
| /// Return plugins taking overrides into account | ||
| pub fn as_plugins(&self, path: &Utf8Path) -> Cow<Plugins> { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
| /// Return plugins taking overrides into account | |
| pub fn as_plugins(&self, path: &Utf8Path) -> Cow<Plugins> { | |
| /// Returns the plugins that should be enabled for the given `path`, taking overrides into account. | |
| pub fn get_plugins_for_path(&self, path: &Utf8Path) -> Cow<Plugins> { |
crates/biome_service/src/settings.rs
Outdated
|
|
||
| for pattern in &self.override_settings.patterns { | ||
| if pattern.is_file_included(path) { | ||
| result.to_mut().0.extend_from_slice(&pattern.plugins.0); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Maybe we should have a Deref implementation on Plugins? Not a fan of those .0 accesses, as it obscures what exactly is being accessed.
CodSpeed Performance ReportMerging #6117 will not alter performanceComparing Summary
|
|
Thanks for help review! I've made some changes accordingly and add a test. Look like there is some snapshot test need to be update, I'll check on those later. |
7fcb3c5 to
e528a3d
Compare
e528a3d to
9c77d21
Compare
arendjr
left a comment
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Thanks a lot!
Related: #5953
Summary
Allow add plugins that runs only in certain files.
Changes
pluginsto overrides settingsas_all_plugins, traverse settings to collect all plugins (including global plugins and all plugins in overrides)as_plugins, takepathas parameter, this method return applicable plugins for the path. (including global plugins and matched plugins in overrides)get_analyzer_plugins, addpluginsas parameter, it no longer return all BiomePlugins stored in cache. Now it only returns BiomePlugins specified in the parameterpluginssettings.as_all_plugins()instead ofsettings.plugins. This ensures all plugins (global + overrides) are all loaded here.fix_file,pull_diagnostics), callsettings.as_plugins(&path)to get list of plugins needed, then pass toget_analyzer_plugins_for_project->plugin_cache.get_analyzer_plugins. This way we'll have the BiomePlugins needed for a file (global + overrides matched by path)Test Plan
Add test to check basic plugins override
crates/biome_service/src/workspace.tests.rs correctly_apply_plugins_in_override