Refactor bootstrap.py script for readability #715
Conversation
consideRatio
force-pushed
the
pr/refactor-bootstrap-for-readability
branch
from
1e746db
to
cf37f58
Compare
consideRatio
force-pushed
the
pr/refactor-bootstrap-for-readability
branch
from
cf37f58
to
0b11304
Compare
consideRatio
force-pushed
the
pr/refactor-bootstrap-for-readability
branch
from
0b11304
to
ba84229
Compare
| """ | ||
| # Support only Ubuntu 18.04+ | ||
| def get_os_release_variable(key): |
There was a problem hiding this comment.
I generally prefer leaving these functions in the top level, as I think that's clearer and we can't accidentally re-use variables from the function inside which this function is defined. Do you prefer having them be nested instead?
There was a problem hiding this comment.
I find this to depend a lot on situations, but what influences my perception that this became more readable and such were.
- Nested function makes it clear it's only used there
- All logic for the bootstrap CLI is in this single file, and that makes me a bit motivated to not have everything declared in the top level scope for some reason. Having this helper function nested under the only function helped me overview the large script files purpose as a whole better, which became quite relevant as the file quite a bit of logical pieces that isn't easy apparent how they relate to each other.
Would you like me to change it back? I have a preference towards having this function either nested or splitting this file into multiple files, but I don't think it's important and can change it back.
There was a problem hiding this comment.
I'd love to change it back! I find extra levels of nesting harder to reason about... If you don't feel too strongly about it I'd appreciate it being changed back.
It's not ideal it's in one file only but it's necessary for our curl based installation...
There was a problem hiding this comment.
I find very nested functions that don't include any indentation are OK, but if there are nested blocks, long comments or long lines leading to indented continued lines, it gets very hard to read. Would prefixing a top-level internal function with _ help make it clearer?
Alternatively would creating a class with static methods work as a form of separation even if it's not strictly necessary?
| @@ -161,20 +191,18 @@ def validate_host(): | |||
| print('The Littlest JupyterHub requires Ubuntu 18.04 or higher') | |||
| sys.exit(1) | |||
|
|
|||
| if sys.version_info < (3, 5): | |||
There was a problem hiding this comment.
We should still print error messages if python is too old, right? Or is this handled somewhere else?
There was a problem hiding this comment.
I think I've updated this correctly in d36c53d, given that we now require ubuntu 18.04+, please check the commit I pushed about this. I don't think we provide an error message elsewhere or similar regarding this.
I deleted this reasoning that you won't have ubuntu18.04+ and Python 3.5 or lower, but maybe and why not have this check still?
bootstrap/bootstrap.py
Outdated
| else: | ||
| logger.info('Setting up hub environment') | ||
| initial_setup = True | ||
| logger.info("TLJH installer isn't installed, installing...") |
There was a problem hiding this comment.
IMO the 'setting up hub environment' is a much more friendly message for a user looking at the logs than for a dev
There was a problem hiding this comment.
I agree! I've pushed a commit about this: b1b867b! What do you think?
|
Thank you for this, @consideRatio! |
I've invested a lot of time to figure out how to test this project against 21.10 and learn about the project in general. By doing that, I made some refactoring work of bootstrap.py to improve my own comprehension.
I'd also want to refactor some other files related to the integration tests, it would help me significantly in order to wrap my head around setting up tests against 21.10.