-
-
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: sopel-plugins (basic version) #1588
Conversation
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.
Couple tiny line notes, but my main point of interest is in the future evolution of cli.utils
.
I'm thinking maybe the formatting bits should go in a subpackage, like cli.utils.formatting
or cli.utils.text
? That would be a nice place to stick things like the ANSI escape-sequence constants—which should probably have all colors added, at least, if not other things too (bold; maybe also underline, etc.).
Also, isn't color 30 black, not gray? 🤔 (And it's not used anywhere, so if you were trying to add only the formatting functions you needed… ya missed one. 😜)
cffac7f
to
e0bf601
Compare
@dgw I changed the
and I use green for enabled plugins, red for disabled one, and yellow for enabled but that can't be loaded properly. All I need now is #1564 to write the documentation. |
Update: actually it needs #1646 to be merged so I can write the doc, then, it can be merged! |
I added a Next step before I can hit the "ready for review", is to implement a |
And now, with |
And now with documentation! 🕺 |
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.
First round! I can't run this through its paces on my CLI just yet, but I will between this and my next review.
This is all little stuff. I wouldn't even bother submitting before I can test the new CLI myself, but 1) I promised a review today and 2) there's a semi-important line note (the first one) that needs to be published anyway.
cb8eb71
to
abcd818
Compare
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.
Since you're clearly working on this (or were very recently), I'll submit the notes I collected today.
I'm sure there will be more to come. Or maybe not… Depends on whether you want to do error-case handling and such in a future PR or just chuck it into this one. :)
Inspired by `apt list`, the `list` action now display the plugin list like this: plugin-name/plugin-type label (source) [status] where: * `plugin-name` is the plugin's name, as used to enable/disable it * `plugin-type` is the type of plugin handler used * `label` is the plugin's label (once loaded) * `source` can be either the python path to the module, or its filename * `status` can be "enabled", "disabled", or "error" (in case of error on loading the plugin) Colors are used on the plugin's name: * green if the plugin is loaded properly and is enabled * red if the plugin is disabled * yellow if the plugin cannot be loaded properly but should be enabled The error status is displayed in red if colors are activated.
Co-Authored-By: dgw <dgw@technobabbl.es>
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.
Seems we're down to nitpick territory. 🎉
I'll open a PR momentarily to take care of sopel-config
being absent from the CLI docs, so we needn't worry about that particular conversation in this PR.
Co-authored-by: dgw <dgw@technobabbl.es>
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 realized that a new PR would mean even more merge conflicts down the road, so I added sopel-config
to the CLI docs myself in this PR, and also fixed the latest round of grammar nitpicks myself. 😁
Should fix #244 and fix #1519 at the same time.
This PR adds a new command line to Sopel:
sopel-plugins
.This command line supports 4 actions:
list
, to list available plugins (all by default, or just enabled/disabled plugins)show
, to show detailed information about a pluginconfigure
, to run a config wizard for a pluginenable
, to enable a plugindisable
, to disable a pluginIn order to implement it, I had to update the plugin handlers, adding methods to get meta description. I think we could built from that to include other meta-data (such as version, author, etc.).
This is the first version of the command. I think it's better to stop here for now, let users test it, play with it, etc. and see what's asked.
In the future, I think we will need to rework how a plugin can expose the commands, URL callback, rules, and jobs to the outside world, but I don't want to do that in that PR, which is big enough as it is, and fairly usable to me.
sopel-plugins list
sopel-plugins show
sopel-plugins configure
sopel-plugins enable
sopel-plugins disable