cli: move sopel.run_script to sopel.cli.run #1493
Conversation
Exirel
force-pushed
the
sopel-cli-run
branch
3 times, most recently
from
e597d36
to
09ebef7
Compare
There was a problem hiding this comment.
You can check the Travis build logs for all the gory details, but to summarize:
- There are unused imports left in
sopel/__init__.py(linting error). - Some of the
cli.runtests fail.
I'll wait until you have a chance to fix the tests to do a more in-depth review.
There was a problem hiding this comment.
Some questions, and a lot of docstring tweaks as usual. I put as many grammar/spelling changes in suggestions as I could, as promised on IRC. 😸
I don't love the duplication of control logic between the legacy parser and the newer subcommands, but it's also probably not worth refactoring out. The legacy mode will only be around for Sopel 7, I think, so the duplicated code will be removed in a year or so.
I do love that this adds the new CLI structure in a transitional state while still keeping the old syntax around. The release notes for Sopel 7 can warn about the upcoming CLI changes, and users can have time to update their service units/scripts/etc.
This makes me glad I merged #1472. 🙌
Thank you! I'm quite happy with how you manage the project, and how you review and comment the PR & issues we send you. It's a happy collaboration so far! |
There was a problem hiding this comment.
With all the bigger issues from my last review taken care of (that I can see—this is a big diff!), I moved on to some nagging questions about the future.
Some of the legacy options have no counterpart in the revamped CLI, but aren't marked as deprecated, which makes me wonder. And one of them (--migrate) has been broken for almost six years! 🤣 Let's weigh the decision on that one in its line note, where I already discussed some pros and cons.
Loved the new, detailed docstrings for stuff like get_configuration! My next questions will be about test coverage of the new CLI (since you updated several tests to just use the legacy parser), so if you want to get ahead of me… 😉 Both old and new should probably be tested, to be fair.
|
Leaving a note for @Exirel upon request: Amend |
Indeed! I would like to mark them as deprecated once there is a proper replacement. For example the list-config option should be replaced when I'll redo PR #1465 (cli: sopel-config), using the work done in this PR. So it's work for later.
Thanks!
Yeah. These tests were added when I reworked the |
|
OK, I kicked Travis to get the PR status reported correctly (for some reason it finished all builds and reported the commit status, but not the PR status). Future work on remaining legacy options like If that's correct, all that's left is for me to sit down and actually run this code on both py2 and py3, when I have time to do that. Assuming that all works, this will get ✔️ Approved and merged as-is. 🎉 |
Yes it is! Now glhf with your manual tests for these changes, it took me some times on my side! |
|
Rebased upon master. Nothing broke, AFAICT, but it might be dangerous to keep this PR open for too long, given its scope. |
You're right about that. I merged it without performing excessive amounts of testing, but we will have plenty of time to fix bugs between now and when Sopel 7 actually ships. 😎 |
|
BOLD MERGE ❤️ |
|
*sees 👀 what you did there* |
Because of the `time.sleep` during the disconnect phase, the signal handling is thrown off and a new bot is spawned before the `bot.hasquit` flag can be checked. Add a check at the top of the loop to see if the `hasquit` flag was set during a disconnect. Fixes sopel-irc#1478. Reapplied because of changes from sopel-irc#1493.
…dling core: reapply part of interrupt fix after #1493 moved things
Related to #1471
This move the
sopel/run_script.pyfile tosopel/cli/run.py, and updates thesopelcommand line tool's interface:sopel startreplaces the default behavior (run the bot)sopel stopreplaces the--quit/--killoptionssopel restartreplaces the--restartoptionsopel configurereplaces the-w/--configure-all/--configure-modulesoptionsThe
-voption for "version" is deprecated,-V/--versionshould be used instead.By default,
sopelworks as before (100% backward compatible), but adds some warning here and there when needed (when using --restart/--quit/--kill for example).This is all thanks to #1472 being merged.