-
-
Notifications
You must be signed in to change notification settings - Fork 402
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
cli.config: implement set
and unset
subcommands
#1986
base: master
Are you sure you want to change the base?
Conversation
Note that an invalid value already present in the config file will block using `sopel-config` commands **at all** for now. This is not new. Plugin-defined `StaticSection`s aren't loaded as such here. Only values present in the config file are parsed by `configparser`, and new values can't be validated unless `cli.utils.load_settings()` is reworked to load plugins' static sections too.
OK, I know I should have review that a long time ago, but I wanted to have an actual opinion, and not just doubt or gut feeling (positive or negative). I apologize because I'm going to ask for more work and it's kind of my fault. So, we both know the problem: at this stage, the command doesn't know the real configuration, i.e. plugins' static sections. We assumed that we couldn't get them, and for my part that's because I thought the config might not be viable at this stage. And I think this is wrong—and partially my fault. When using Now, what if "hey I manually edited and now it's not valid what do I do?"... then I'll say: add a flag to deactivate validation (by skipping loading I guess?). What do you say? |
What I'd like is a way to load only the config sections of plugins, but (correct me if I'm forgetting something) that's not possible with the current loader architecture, plugin structure, nor how Python loads things in general. AFAICT we're stuck loading all plugins and running their So with that out of the way, yes. I agree in principle that I'd also like to handle the case where In any case, I think this PR is best left alone for now and pushed to 8.0. Agree? (You can re-milestone and assign me if you do. 😁) * — So I did think of an alternative: Call each plugin's |
Yup, that's the problem. I'd like to fix it too, but that's not currently possible without changes. And even then, the solution won't be perfect because we can't prevent someone from defining sections from the
Got the same idea, with the same conclusion. What I want to do in a future version, is to separate the steps:
Get metadata: that could be a way to inform about the plugins without having to run any more code than necessary, i.e. something that returns the author, short & long description, and a list of (section name, config class), which could come in handy. But that's future talk! |
This should have been dropped back to draft status weeks ago. Or really, about 5 months back. |
Setting some 8.0 priorities. This can go in a minor 8.x release, or 9.0, depending. It's a new feature with no accompanying deprecations that shouldn't affect the behavior of anything else. |
Description
If @Exirel can submit PRs that weren't "requested" by an issue, then so can I! 😛
Honestly, this was going to happen sooner or later. It's the next natural evolution of the
sopel-config
CLI.Why now? I was working on an out-of-tree plugin and kept needing to edit the config file due to shortcomings in the configuration wizard UX (namely, you can't unset a value through a prompt in
sopel-plugins configure <plugin_name>
).While I've never been terribly fond of how the configuration wizard works—goodness knows it needs an overhaul—fixing that was too big of a project to start late on a Sunday night. This, by comparison, took less than half an hour to build out and (manually) test. Since we don't have any unit tests for large portions of the CLI module (other than
cli.run
andcli.utils
), I didn't worry about adding any for these new subcommands just yet.Checklist
make qa
(runsmake quality
andmake test
)