Disable nbextension in first location found #2725
Conversation
|
I like this better than #2724. It's what I would expect from disable. I would also expect |
notebook/nbextensions.py
Outdated
| }}, "Disable an extension from a Python package name") | ||
| } | ||
| flags['python'] = flags['py'] | ||
| aliases = {'section': 'ToggleNBExtensionApp.section'} |
There was a problem hiding this comment.
This should still inherit the --system, --user, --sys-prefix flags, etc. from ToggleNBExtensionApp. There's still a valid use case:
A system-wide installation enabled an extension, but you want to disable it in your own configuration. Passing --user in this case (which should store False, not pop config) makes sense as an override of the system defaults.
There was a problem hiding this comment.
I've added the --user and --sys.prefix flags. I don't think --system is necessary, since there's nothing for system config to override.
|
I was in the process of writing a comment that it would be nice to not lose the inheritance then saw that you had just updated it to do exactly what I was going to ask for :) |
|
Oops. I was confused - the diff just moved because you added the uninstall application. Restoring comment: It would probably be nice if we can keep the extensionapp inheritance, if only for preserving the consistency of the flags/aliases without having to duplicate it all. All of the enable flags should also be on disable (--system is missing, as is --debug). |
| except ArgumentConflict as e: | ||
| sys.exit(str(e)) | ||
| else: | ||
| self.find_uninstall_extension() |
notebook/nbextensions.py
Outdated
| help="""Which config section to disable the extension in.""" | ||
| ) | ||
|
|
||
| def start(self): |
There was a problem hiding this comment.
Could we get away with only overriding start, instead of losing the ToggleNBExtensionApp inheritance?
|
I decided there wasn't enough significant code in common between the enable & disable apps to keep a base class, so I folded the remainder of ToggleNBExtensionApp into EnableNBExtensionApp and simplified it. |
|
I'd like to publish a release candidate by EOD so that people can start testing 5.1 this weekend and then try to officially release on Tuesday. Does this PR close #2149 or is it a part of the solution? From my understanding, with or without an |
|
It doesn't add a I'd consider saving the It shouldn't be much more work to do it for serverextensions, but there's not much of the day left here now. I'll have a go after dinner, but I might not get it done today. |
|
Ok, no worries. Let's shoot for a rc tomorrow! |
|
I think the find-and-remove strategy makes sense for uninstall, but on reflection, I wonder if the default action for disable should be to set it to False in the user config directory, overriding system or installed config. In the scenario Min described - extension enabled systemwide, user wants to disable it - running |
|
In fact, what I'm suggesting is what |
That sounds great to me, actually. |
Closes jupytergh-2725 (see discussion on that PR)
|
#2729 does that. Closing this one. |
Alternative to #2724.
This implements option 1 from Min's comment: find the first location where the config enables the extension, and remove it. This makes it the default, and simplifies the options available, on the grounds that 99% of the time, when you're asking to disable something, you don't care which bit of config is enabling it.
It adds some code because it bypasses the inheritance tree of applications that has been built. Looking at this code, I think we've over-abstracted the actual operations a bit, which makes it less clear what's going on. I'd like to flatten it a bit, but not in this PR.