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

cli: sopel-module #1434

Closed
wants to merge 18 commits into from
Closed

cli: sopel-module #1434

wants to merge 18 commits into from

Conversation

Exirel
Copy link
Contributor

@Exirel Exirel commented Jan 3, 2019

Disclaimer: this is an experimental command line tool. I suggest to merge it as-is, and to iterate in further PR if we need to. I've been working on that for few days, and I think now it is in a good enough shape to work properly.


So last day I was working on #1424 and #1429 and I was wondering: how can I get the list of available/enabled modules for Sopel? So I look up the CLI (sopel or python sopel.py) with no result.

So... maybe I could do something about that? After all, there is an old issue about module enabled/excluded (#244), and maybe I can suggest a solution.

And here it is: a Pull Request that adds a new command line interface: sopel-module.


Entry point

At the moment, I added a sopel-module entry point in the setup.py configuration.
Maybe later we'll need to add a .py file at the root of this repository, like there is to launch python sopel.py from the source.

That'll be for later.

Code organization

I don't like having a sopel.run_script module. I think, if we want to add multiple CLI for sopel, we should have a proper sub-package for that.

So I put all my code into sopel.cli, and the function used as entry point is main.

I think this structure could be use for the run_script code too, but I don't want to change that in that PR: it'll be for another one, once this is merged.

CLI

To test it from the source:

python setup.py develop

This will install both entry point sopel and sopel-module in your virtualenv, and then you can do:

sopel-module list
sopel-module show xkcd
sopel-module disable xkcd
sopel-module enable xkcd

Good luck with the review!

list

The command sopel-module list enumerate modules, as Sopel does when looking for modules.

usage: sopel-module list [-h] [-c filename] [-p] [-t] [-a | -e]

List availables sopel modules

optional arguments:
  -h, --help            show this help message and exit
  -c filename, --config filename
                        Use a specific configuration file
  -p, --path            Show the path to the module file
  -t, --type            Show the type to the module file: `p` for package
                        directory, `m` for module file, `?` for unknown
  -a, --all             Show all available module, enabled or not
  -e, --excluded        Show only excluded module

show

The sopel-module show <module> command display various information about the module (if found): it tries to list commands, rules pattern, events and intents, that will be loaded by the module.

usage: sopel-module show [-h] [-c filename] module

Show a sopel module's details

positional arguments:
  module

optional arguments:
  -h, --help            show this help message and exit
  -c filename, --config filename
                        Use a specific configuration file

enable

The sopel-module enable <module> tries to enable a module:

  • if there is a whitelist, it adds the module to it,
  • it always try to remove it from excluded,
usage: sopel-module enable [-h] [-c filename] module

Enable a sopel module

positional arguments:
  module

optional arguments:
  -h, --help            show this help message and exit
  -c filename, --config filename
                        Use a specific configuration file

If there is no whitelist yet, this command won't use it. Maybe an option could be added later for that - I suggest to keep it simple for now, and to iterate in further PRs.

disable

The sopel-module disable <module> tries to disable a module by adding it to the list of excluded.

usage: sopel-module disable [-h] [-c filename] module

Disable a sopel module

positional arguments:
  module

optional arguments:
  -h, --help            show this help message and exit
  -c filename, --config filename
                        Use a specific configuration file

@dgw
Copy link
Member

dgw commented Jan 9, 2019

I'm really enjoying watching you have fun building this new interface, but I'm curious about one point:

How does this work with multiple config files? (Or rather, how do you plan to make it do so?)

@Exirel
Copy link
Contributor Author

Exirel commented Jan 9, 2019

How does this work with multiple config files?

I'll add the same options used by sopel, ie. -c/--config, so one can choose the configuration file. I wanted to do the core feature first, and add these "global options" later. I might add --quiet / --verbose options too.

I may want to change the messages, too: after using enable/disable, operator needs to restart the sopel instance using this configuration file, so a warning line could be welcome.

Oh well, we'll see. This PR is open for comments!

@Exirel
Copy link
Contributor Author

Exirel commented Jan 10, 2019

Ok, since I'm quite happy with the current state of this PR, I added the -c option:

(sopel) $ sopel-module list -c toto
Unable to find the configuration file /home/sopel/.sopel/toto

I added the option for each sub-command, otherwise you have to do sopel-module -c path/config list which is ugly for the user.

Even if I think there are room for improvement (the list output is not the best), I believe this PR could take advantage of other refactoring regarding the module loading and the config loading.

This experiment, so far, show that:

  • there is no clear way to describe a sopel module, but to check for each of its functions,
  • modules don't have clear attributes of their own,
  • the internal API to load modules and configurations have room for improvement to be fully reusable,
  • the exclude config is easy to use, but the enable list has some weird behavior that might be difficult to grasp as a end-user.

I would say it's expected for such old software with a lot of built-in modules, with a low focus on third-party modules and tools. Nothing impossible to fix, and many little things are possible to be done for the 7.0 release!

@dgw dgw added this to the 7.0.0 milestone Jan 10, 2019
@dgw dgw added the Feature label Jan 10, 2019
@Exirel Exirel changed the title WIP/Experimental: sopel-module CLI cli: sopel-module Jan 24, 2019
@Exirel
Copy link
Contributor Author

Exirel commented Jan 24, 2019

@dgw I updated my description, and removed obsolete comments in the PR. I think it's safe to have a look now. It's ready to be merged, and I'd love to work on the next step. That is to say: moving sopel.run_script into sopel.cli.run.

So yeah, it was fun to work on that but now I can't do much more without refactoring too many code. :p

@Exirel Exirel mentioned this pull request Jan 30, 2019
Copy link
Member

@dgw dgw left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

So far, there's one user-facing thing (sopel-module list shows __init__.py) that would be best fixed in a separate rewrite of the module-finding API that this CLI calls. We discussed this already on IRC.

I'm playing with it first, then I'll look at the code for any obvious things.

Edit: As mentioned on IRC, there's a case to be made for rewriting some of the module logic before coming back to this. Things like packaged modules with several files seem to throw this CLI off, and it's probably shortcomings in the underlying internal functions doing that.

@Exirel
Copy link
Contributor Author

Exirel commented Feb 6, 2019

As mentioned on IRC, there's a case to be made for rewriting some of the module logic before coming back to this.

Let's work on that first then! I'll close this PR for now.

@Exirel Exirel closed this Feb 6, 2019
@dgw dgw removed this from the 7.0.0 milestone Feb 8, 2019
@Exirel Exirel deleted the cli-sopel-module branch September 5, 2019 09:44
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants