-
Notifications
You must be signed in to change notification settings - Fork 339
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
Refactor bootstrap.py script for readability #715
Refactor bootstrap.py script for readability #715
Conversation
1e746db
to
cf37f58
Compare
cf37f58
to
0b11304
Compare
0b11304
to
ba84229
Compare
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Minor questions, happy to merge after
""" | ||
# Support only Ubuntu 18.04+ | ||
def get_os_release_variable(key): |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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.