run_script: clean up & reorganize code #1429
Conversation
Right you are, because of timing and because this is a fairly involved change. I'm honestly not even going to look at the changes in this PR until 6.6.0 is out—hope you don't mind! |
|
I don't mind at all! I have time right now, but that's just me, and I don't expect you to look at it at the moment, or even to comment - so no worry here! Just a quick note: I didn't change any functionnality, it's only a code refactoring, where I took block of code and extracted them into function. The big change is in 512e8f5 and from a user perspective, it changes nothing. Hey, by the way: happy new year! ;-) |
Exirel
force-pushed
the
run-script-clean-up
branch
from
d1f2478 to
8958997
Compare
Exirel
force-pushed
the
run-script-clean-up
branch
from
d6e4017 to
686993c
Compare
There was a problem hiding this comment.
Before you see the line-comment count and have an anxiety attack: Most of this is tiny grammar tweaks.
GitHub only supports suggesting changes one line at a time (lame). Combine that with the number of little tweaks I wanted to make in both new and existing docstrings/comments, and you get 31 line notes.
You can fix probably 26 of them in a few minutes total. Only maybe 2 or 3 of the five remaining notes are actual questions that might take a moment to answer. 😺
TL;DR: global variables make testing difficult and reduce reusability The function `enumerate_configs` now takes two arguments: * a required argument, the directory to look for configuration files * an optional argument, the extension (dot-included) It works as before otherwise, with added docstrings for clarity. Since this function isn't used outside `sopel.run_script`, we should be fine.
Exirel
force-pushed
the
run-script-clean-up
branch
from
686993c to
0c87de6
Compare
TL;DR: global variables make testing difficult and reduce reusability The function `find_config` now takes three arguments: * two required arguments: the directory to look for configuration, and the configuration filename to look for * an optional argument, the extension (dot-included) It works as before otherwise, with added docstrings for clarity. Since this function isn't used outside `sopel.run_script`, we should be fine. I also changed a convoluted `if` to make it simpler to understand without changing its behavior.
The `sopel.run_script.homedir` variable was a "global" variable that belonged to the configuration realm. Therefore, I moved it to the `sopel.config` module, and renamed it to `DEFAULT_HOMEDIR`. This name: * follow the PEP8 naming convention (not required but nice to have) * move from a "run thing" module to a "config" module, which is more appropriate * express the need for being the "default" location for configuration, since a local configuration can be used that might not be in it All of this is unecessary, and more "nice to have", but it could help fixing sopel-irc#1243 - if that makes sense.
When you install Sopel using `pip install` (or `setup.py develop`), it
installs a script, that contains this interesting bit:
sys.exit(
load_entry_point('sopel', 'console_scripts', 'sopel')()
)
Wich is the same as:
from sopel.run_script import main
sys.exit(main())
So I replaced all `sys.exit(n)` in `run_script` by `return n` instead.
It works exactly the same, but it's easier to test or interact with
outside of the pre-defined command line tool. This is a step forward
helping other to reuse Sopel's code in their own way.
Exirel
force-pushed
the
run-script-clean-up
branch
from
0c87de6 to
d9469e5
Compare
|
@dgw I think it's good now! :) |
When the bot exits due to an error, its exit code must not be 0, so we return a non-0 value. By default, it's 1, but to prevent systemd to restart the bot in some specific error cases, we return 2 instead. For that purpose, we use 2 variables: * `ERR_CODE` (1), the default error code * `ERR_CODE_NO_RESTART` (2), the error code to be returned when the program can not be restarted without a modification This way, these values may change in the future (even if it is very unlikely) without breaking the code, and more importantly, this behavior is now easier to document (with docstrings and clear names).
Exirel
force-pushed
the
run-script-clean-up
branch
from
5465d9b to
f4ce4a7
Compare
New global added in sopel-irc#1429 is a no-brainer to use in these spots.
My end goal is to address issues like #1243 or #1175 and for that the code must allow it, either by being more modular, or more reusable, or more testable - not mentioning documentation, which is not an easy task at all.
In this PR:
sopel.run_scriptinto separated and testable functions,sys.exitandos._exitcall, while keeping the exit value for the command line,so it still lacks proper unit-testing and documentation, but in my opinion it is a good starting point.
That being said, I think this is a good topic for Sopel 7.0.0 or another future version.