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

Add "all" flag to nbextension and serverextension commands #2715

Closed
wants to merge 2 commits into from

Conversation

gnestor
Copy link
Contributor

@gnestor gnestor commented Aug 1, 2017

@takluyver @minrk I'm not sure whether this is the right approach... As opposed to "check every level where the enable config is defined", this is just adding an "all" trait to the top-level class and some logic to the start method that will set user and sys_prefix to True if the all flag is enabled. This requires all subclass instances to call super(CommandInstance, self).start().

Closes #2149

@gnestor gnestor requested review from takluyver and minrk August 1, 2017 16:42
@gnestor gnestor modified the milestones: 5.2, 5.1 Aug 1, 2017
@gnestor gnestor mentioned this pull request Aug 2, 2017
11 tasks
@takluyver
Copy link
Member

I think it's just going to end up raising this error in some cases, and not doing what you expect in others. The machinery is designed to select one location to make a change, not a set of locations.

Rather than trying to add this for all commands, I'd focus on the disable command, and maybe uninstall. It doesn't make much sense to install or enable an extension in all locations.

There are questions over what it should do if you don't have write permissions to all the directories it wants to change (there are system config locations, so that will be common if you run it as a regular user). Should it always error out in that case, or only if it would change a non-writable file? Should it fail quickly, or make any changes it can before throwing an error?

@minrk
Copy link
Member

minrk commented Aug 2, 2017

I don't think setting sys_prefix and user both to True will do the right thing, because only one is triggered in the end, unless something changed. I thought this would through a mutually-exclusive flag error.

I think you'll need to handle all specially, and I see two options:

  1. Follow pip's example, and remove the first one you see in the priority order.
  2. go through them all in order.

Option 1. seems like the more logical behavior, to me. It has the potential disadvantage (like pip) that you have to re-run it until it shows nothing installed to be sure that it's truly gone if you have something enabled/installed at multiple levels. But it has the advantage that it only does one action.

Option 2. might better address the specific use case @ellisonbg brings up in #2149, where the real goal is "scrub every trace of this and start from scratch". However, 'disable' is not quite the right action for that use case, because disable sets an explicit False flag (thus overriding lower-priority config), whereas scrubbing every trace should pop values from config, not set them to False. That makes "remove every last trace of this" a somewhat distinct action from disable.

This ambiguity doesn't affect uninstall, though.

It seems like what would be really nice is if uninstalling an extension also removed enabling configuration, but that's getting pretty complicated.

Per @takluyver's comment about errors, I think it should raise any PermissionErrors it encounters.

@takluyver
Copy link
Member

I think you'll need to handle all specially, and I see two options:

  1. Follow pip's example, and remove the first one you see in the priority order.

I've had a go at this in #2725 .

  1. go through them all in order.

And this in #2724 .

@gnestor
Copy link
Contributor Author

gnestor commented Aug 4, 2017

Thanks @takluyver! I'm going to close this one...

@gnestor gnestor closed this Aug 4, 2017
@gnestor gnestor deleted the extensions-all branch August 4, 2017 12:39
@github-actions github-actions bot locked as resolved and limited conversation to collaborators Apr 6, 2021
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Add nbextension/serverextension commands for uninstall/disable all
3 participants