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

Rework internal APIs for Command Line Tools #1471

Closed
Exirel opened this issue Feb 6, 2019 · 21 comments
Closed

Rework internal APIs for Command Line Tools #1471

Exirel opened this issue Feb 6, 2019 · 21 comments
Labels
Milestone

Comments

@Exirel
Copy link
Contributor

Exirel commented Feb 6, 2019

After working on sopel.run_script and some internal features regarding configuration and modules loadings, it appears the internal APIs require a bit of love and care. There is a lot going on with that, so I'll try to list the things I think about:

  • move CLI related code to sopel.cli.* sub-package,
  • replace sopel.run_script by sopel.cli.run and adapt the setup.py's entry point accordingly,
  • centralize how configuration files are found and loaded,
  • centralize common CLI options (like -c config),

and I'm sure I'm forgetting some part.

For the record, I personaly worked on these PR before: #1429 #1434 #1445 #1465 - and others have worked toward the same goal (PR #1464 is a good example of that). Some PR have already been merged and some are closed until this issue is fixed.

Note: this is not an issue to discuss where the config files must be, or how to load modules. This is about making sure all the good ideas around how to structure internals APIs are shared.

@Exirel Exirel mentioned this issue Feb 6, 2019
@HumorBaby
Copy link
Contributor

Feature request: allow for configuration of a specific module (as opposed to the current --configure-modules flag that asks y/n for each possible module).

Let me know if you make an "open call for patches" regarding this, and I will gladly contribute what I can :)

@Exirel
Copy link
Contributor Author

Exirel commented Feb 6, 2019

Feature request: allow for configuration of a specific module

Hm. In #1465 I played with the idea of a sopel-config init, and something like sopel-module configure <module> or sopel-config module <mod-name> or something similar might work.

For that, I think we need to make sure all Configuration Wizards comply with the configuration loading, and that we can find the module as Sopel does when running it.

@HumorBaby
Copy link
Contributor

Ah! I see now that's what ... init was meant to do.

@Exirel
Copy link
Contributor Author

Exirel commented Feb 9, 2019

After a review of @dgw about add_config_arguments renamed add_common_arguments, I was thinking that there are 2 common options:

  • -v/--version: show the program's version
  • --verbose and --quiet: more/less output on stdout

@dgw
Copy link
Member

dgw commented Feb 9, 2019

@Exirel It might be more useful to have only long-form for --version and reserve the short args -v/-q as a pair for verbose/quiet.

We should (IMO) reassign -v and give --version the short form of -V instead. After adequate warning, of course.

(I lied in my original comment. I did have time to check the website listing of CLI args after all!)

@Exirel
Copy link
Contributor Author

Exirel commented Feb 12, 2019

@dgw I fully agree @ --version/-V

@Exirel
Copy link
Contributor Author

Exirel commented Feb 26, 2019

I was thinking about the run_script.py arguments: for actions like "restart" or "quit", it uses mutually exclusive options : --restart and --kill can not be used together.

In that case, I would change the command line from "options" to "arguments", like this:

$ sopel [action] [options]

Where action is run by default, and options is the list of common options + action-specific options.

This would be used like this:

$ sopel
$ sopel run --daemonize
$ sopel restart
$ sopel quit
$ sopel kill
$ sopel list-config
$ sopel wizard
$ sopel wizard --modules

For the record, these options are used in this order:

  • --kill
  • --quit
  • --restart

Release Cycle

In 7.x I would:

  • add the "action" argument, default to the "legacy" mode (no UX change)
  • display a warning when options like --restart are used in legacy mode

In 8.x I would:

  • change the default behavior from "legacy" to "run"
  • remove the legacy mode completely

@dgw
Copy link
Member

dgw commented Feb 26, 2019

SGTM tbh. I have nothing to add, except that I like the "action" proposal.

@Exirel
Copy link
Contributor Author

Exirel commented Feb 28, 2019

I like the "action" proposal.

I need #1472 to be merged first then. :D

@dgw
Copy link
Member

dgw commented Feb 28, 2019

I need #1472 to be merged first then. :D

Ask and ye shall receive! :D

@Exirel
Copy link
Contributor Author

Exirel commented Mar 19, 2019

Since #1472 has been merged, I was able to work on:

I'm quite happy with how #1472 turned out, and how it helps to add new features, and how cleaner it is to work for existing and new CLI tools.

Happy me!

Edit: I didn't use 1489 instead of 1493 here. That's fake news.

@dgw
Copy link
Member

dgw commented Mar 20, 2019

#1489 to move sopel.run_script in sopel.cli.run

Ah, that's #1493 isn't it?

Nothing to see here but FAKE NEWS…

@dgw
Copy link
Member

dgw commented Apr 6, 2019

With #1235 merged, what are the chances of making that easier to configure in future enhancements to the sopel-config CLI? It's a weird one, because it directly creates a config section per-channel and puts literal Python data structures (dict containing lists) in the values…

The selection interface you'll presumably implement to resolve #244 could be reused for choosing channels to configure, and picking modules/methods to disable.

Draft documentation for the feature is here.

@Exirel
Copy link
Contributor Author

Exirel commented Apr 6, 2019

I think that would be more related to a sopel-plugins CLI, as in:

$ sopel-plugins disable-channel <modname>
$ sopel-plugins disable-channel <modname> <commandname>
$ sopel-plugins allow-channel <modname>
$ sopel-plugins allow-channel <modname> <commandname>

Or something alike. I don't know yet. But I like the {disable|allow}-channel stuff.

@Exirel
Copy link
Contributor Author

Exirel commented Apr 6, 2019

Oh and by the way, that would require the new plugin interface, so it would be much easier to get a list of plugins, and from that list, to extract the available commands.

@dgw
Copy link
Member

dgw commented Apr 6, 2019

I think that would be more related to a sopel-plugins CLI

Fine with me, if there's other stuff to put there besides this one feature.

would require the new plugin interface

Is that (#1479) actually still WIP, or should I try to review it this week(end)?

@dgw
Copy link
Member

dgw commented Apr 6, 2019

This said:

I think that would be more related to a sopel-plugins CLI

Fine with me, if there's other stuff to put there besides this one feature.

I still think (en|dis)abling plugins on a per-channel basis is very much "config" related. I thought sopel-plugins would be more about managing plugins themselves, i.e. install/remove/search.

@Exirel
Copy link
Contributor Author

Exirel commented Apr 6, 2019

Tough choice. I don't have the right argument yet, so I'll leave that for future me. I'm sure we'll figure out something meaningful and so obvious we'll feel bad not to find it sooner.

At the moment, #1479 is still a WIP, I want to fix something - that'll have to wait next week.

@Exirel
Copy link
Contributor Author

Exirel commented May 12, 2019

Yay, #1479 got merged, and new things are coming in.

At the moment, I just pushed #1598 to allow such command: sopel start --config-dir /etc/sopel, which will look at /etc/sopel/default.cfg for its config file.

@dgw
Copy link
Member

dgw commented Nov 21, 2019

@Exirel Have you actually done everything you set out to do with this?

@Exirel
Copy link
Contributor Author

Exirel commented Nov 21, 2019

Actually... yes, I think I did. It could always be better, but I don't think it's worth keeping that reminder in particular. I'd rather see new ideas and improvement in their own issues when they come by.

@Exirel Exirel closed this as completed Nov 21, 2019
@Exirel Exirel added this to the 7.0.0 milestone Nov 21, 2019
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
None yet
Development

No branches or pull requests

3 participants