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.

Sign up for GitHub

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

Jump to bottom

Support optional plugins #3193

Merged
merged 13 commits into from
Dec 11, 2022
Merged

Support optional plugins #3193

merged 13 commits into from
Dec 11, 2022

Conversation

pepeiborra
Copy link
Collaborator

@pepeiborra pepeiborra commented Sep 18, 2022 •
edited by gitpod-io bot
Loading

Adds support for plugins that are not enabled by default

@pepeiborra pepeiborra force-pushed the optional-plugins branch 2 times, most recently from c5a605c to 6cee21e Compare September 18, 2022 14:46
@michaelpj
Copy link
Collaborator

This feels a little special-case-y. Doesn't the Properties machinery have a way to let the plugin set default values for its configuration? Could we extend that to cover the generic options somehow? @berberman ?

@pepeiborra
Copy link
Collaborator Author

This feels a little special-case-y. Doesn't the Properties machinery have a way to let the plugin set default values for its configuration? Could we extend that to cover the generic options somehow? @berberman ?

The Properties machinery is for plugin-specific options. For generic options, I think you will struggle to find a better way than this. What in it feels special-case-y?

@michaelpj
Copy link
Collaborator

What in it feels special-case-y?

In that we have to add a new special field to plugin descriptors just to control the default value of one of the generic plugin options. What if we want to, say, have a plugin for which code lenses are off by default? Add another field? Then again, it's not the end of the world to generalize later...

@pepeiborra
Copy link
Collaborator Author

In that we have to add a new special field to plugin descriptors just to control the default value of one of the generic plugin options. What if we want to, say, have a plugin for which code lenses are off by default? Add another field? Then again, it's not the end of the world to generalize later...

That's a good point, I have generalised the scheme to cover all the generic options.

@pepeiborra pepeiborra marked this pull request as ready for review September 18, 2022 19:35
@pepeiborra pepeiborra mentioned this pull request Sep 22, 2022
Closed
@pepeiborra pepeiborra requested review from fendor and wz1000 and removed request for jneira, uhbif19, eddiemundo, berberman and OliverMadine September 22, 2022 13:25
@pepeiborra
Copy link
Collaborator Author

Third attempt, let's see if this is the winner.

@pepeiborra pepeiborra enabled auto-merge (squash) September 23, 2022 05:42
@pepeiborra
Copy link
Collaborator Author

It would be good to get this landed while I still remember the details...

michaelpj
michaelpj approved these changes Oct 24, 2022
View reviewed changes
Copy link
Collaborator

@michaelpj michaelpj left a comment

Choose a reason for hiding this comment

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

Sorry for taking so long to look at this. LGTM, mostly just some documentation nits.

hls-plugin-api/src/Ide/Plugin/Config.hs Show resolved Hide resolved
hls-plugin-api/src/Ide/Plugin/Config.hs Outdated Show resolved Hide resolved
hls-plugin-api/src/Ide/Plugin/Config.hs Outdated Show resolved Hide resolved
hls-plugin-api/src/Ide/Plugin/ConfigUtils.hs Outdated Show resolved Hide resolved
hls-plugin-api/src/Ide/Types.hs Outdated Show resolved Hide resolved
hls-plugin-api/src/Ide/Types.hs Outdated Show resolved Hide resolved
hls-plugin-api/src/Ide/Types.hs Outdated Show resolved Hide resolved
hls-test-utils/src/Test/Hls.hs Outdated Show resolved Hide resolved
@pepeiborra
Copy link
Collaborator Author

Sorry for taking so long to look at this. LGTM, mostly just some documentation nits.

Thanks for the review, I'll get to your comments in a couple of weeks

@pepeiborra
Support initial generic config for plugins
a0d18e6 
This enables opt-in  plugins, i.e. plugins that are not enabled by default
@pepeiborra
Support initial generic config for plugins
a586121 
This enables opt-in  plugins, i.e. plugins that are not enabled by default
@pepeiborra
Address review comments and remove optionality in configInitialGeneri…
62506c4 
…cConfig

I can't remember why this was optional in the first place, which probably means no good reason for it to be
fendor
fendor approved these changes Nov 20, 2022
View reviewed changes
Copy link
Collaborator

@fendor fendor left a comment

Choose a reason for hiding this comment

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

LGTM!

hls-plugin-api/src/Ide/Plugin/ConfigUtils.hs Outdated
Comment on lines 63 to 64
genericDefaultConfig = case configInitialGenericConfig of
config ->
Copy link
Collaborator

Choose a reason for hiding this comment

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

Suggested change
genericDefaultConfig = case configInitialGenericConfig of
config ->
genericDefaultConfig =

@pepeiborra
use defConfig instead of def
a52790c
@pepeiborra
test
f546b00
@pepeiborra
descriptor is the golden source for plugin default configs
28ce8d4 
Read plugin defaults from plugin descriptor instead of copying into initial config

This is less fragile as now the initial generic config specified in the plugin
descriptor is enforced by construction.

The downside is that now the descriptor is needed to parse the config, which
means a few places where a plugin id was enough now require a plugin descriptor,
which is much harder to get. We paper over this by moving these call sites to
the Action monad.
@pepeiborra
Merge remote-tracking branch 'origin/master' into optional-plugins
403b5de
@pepeiborra
redundant import
aeab335
@pepeiborra
Copy link
Collaborator Author

I found a problem with this scheme: if the user specifies config for any plugin, HLS will forget all the initial defaults. This would happen as follows:

  1. at startup we seed the plugin config, a map of plugin configs, with the initial values for each plugin.
  2. at runtime the user custom config fills this map with one entry containing custom config for a specific plugin
  3. HLS will parse the user custom config, forgetting the previous value with all the initial plugin configs

I added some tests to track this issue. To make the tests pass, I first considered merging the parsed plugin config map with the previous config map. I ended up implementing a slightly different solution:

  • no longer initialise the plugin config map with the initial values
  • when resolving the current plugin config, read from the plugin map and fall back to the initial defaults if no entry

I like this solution better because it no longer required initialising the config with the plugin defaults. The downside is that to read the current plugin config, we now need the plugin descriptor, not just the plugin id. This required a bunch of additional changes in plugin code.

@pepeiborra pepeiborra merged commit 6f5a735 into master Dec 11, 2022
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@fendor fendor fendor approved these changes

@michaelpj michaelpj michaelpj approved these changes

@wz1000 wz1000 Awaiting requested review from wz1000 wz1000 is a code owner

@isovector isovector Awaiting requested review from isovector

@Ailrun Ailrun Awaiting requested review from Ailrun

@georgefst georgefst Awaiting requested review from georgefst georgefst is a code owner

Assignees
No one assigned
Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
3 participants
@pepeiborra @michaelpj @fendor