-
-
Notifications
You must be signed in to change notification settings - Fork 3.4k
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
Block fetching the settings for a plugin that is disabled. #7147
Conversation
Thanks for making a pull request to JupyterLab! To try out this branch on binder, follow this link: |
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.
Looks good, one minor comment inline.
While this is clearly a step in the right direction, I also start asking the question about what would happen if a transforming plugin has autostart: false
, or is deferred, and is not required by another plugin. Since the transformers are typically (always?) registered in the activate
function, I assume they would display the same behavior. I'm happy to leave the decision of whether to punt this or not to @afshin .
I'm not 100% convinced this is the right way to do this at all. Here are some of my concerns:
|
As for your question about transformers, they happen in the I am conflicted about this approach because perhaps it is legitimate to disable a plugin but still load its preferences and use them. We need to have a coherent rationale about this. That's why I've left this as a work in progress, because I'd like to raise the issue in our meeting today. |
How so? If
Sure, let's try to brain storm some alternative solutions in today's call (written proposals also accepted 😉 ). |
Yeah I suppose we could add a |
I think the Linux Usage failure is legitimate because I'm modifying the |
Okay, I think the reason that this check fails: python -m jupyterlab.browser_check --core-mode Is because it pulls |
I am unsure what is happening here. The Linux usage test times out so I can't see log output. When I run the individual stages of the Linux Usage manually following along with Does anyone have any ideas how to un-stuck this? |
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.
@afshin Sorry for being slow in reviewing this. This mostly looks good now, but I have some questions below.
} | ||
|
||
async list( | ||
query: 'active' | 'all' = 'all' |
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.
Would it make sense to move this functionality to the services
package itself? It seems as if it might have reuse value? That might also formalize it more, as we can add a docstring explaining that active
implies that it is not disabled, and not deferred.
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.
I thought about putting it in services, but it seemed very specific to the functionality of the SettingRegistry
and it seemed to me that the services package should be completely agnostic about what you're doing with the data being returned from the back-end.
Another place I considered putting this logic was as a query parameter to the back-end, which still remains a possibility (in which case, I'd agree with your suggestion to move this into services).
But for now, this seemed like the most unobtrusive thing to do because it seemed to localize the changes to the actual location of the concern. Do you disagree?
}); | ||
|
||
// If there are plugins that have schemas that are not in the setting | ||
// registry after the application has restored, try to load them manually. |
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.
Just so we're clear: what is the purpose of the code that follows? This comment says what is happening, but not why.
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.
Basically, if a plugin is deferred but not disabled and nobody has chosen to use it, its settings will not be editable by end users. This logic is meant to handle that scenario. It also handles the problem you originally pointed out if a deferred plugin has a transformation that never completes.
…instead of filtering via the service manager.
…sabled bug for individual plugins.
…lugin settings logic in a comment.
followup #7147: adds SettingConnector.save, reenables saving of user settings
References
Fixes #7107
As a side-effect of this work, I proposed we remove the concept ofdeferredExtensions
for JupyterLab 2.0 on the weekly call.Dependency: #7216
Code changes
index.js
to make sure extensions that export a single plugin do not fall through the cracks and respect disabling/deferring.deferredExtensions
anddisabledExtensions
when it preloads the setting registry.app.restored
and then attempts to load the settings for any plugins that did not preload, includingdeferredExtensions
.jupyter.lab.transform
flag to true, there is a console warning thatautoStart
may have been set tofalse
or that the plugin may be one of thedeferredExtensions
.User-facing changes
N/A
Backwards-incompatible changes
N/A