refactor: MAJOR refactor/beautify of build.sh#632
Conversation
|
i think i need to find a better use of my spare time x`) ... |
| shift 2 | ||
| ;; | ||
| *) | ||
| shift |
There was a problem hiding this comment.
Let's be a little more defensive here and error out instead of silently ignoring invalid options
| *) | ||
| shift | ||
| ;; | ||
| esac |
| set -Eeou pipefail | ||
|
|
||
| # color codes | ||
| # fold me :) (in vscode vscodium etc) |
There was a problem hiding this comment.
I don't really see the point of a "fold me" comment. Even more so a reference to one particular editor. I don't think this is helpful
Frankly I find the region/end region comments more distracting than helpful. Please remove
|
|
||
|
|
||
| # Function to install dependencies for most Linux systems | ||
| install_dependencies_for_most_linux_systems() { |
There was a problem hiding this comment.
Can we just call this install_dependencies please
|
|
||
| # Function to install dependencies for most Linux systems | ||
| install_dependencies_for_most_linux_systems() { | ||
| echo_success "Checking dependencies" |
There was a problem hiding this comment.
Maybe the detected xyz could be the success?!
| sphinx-build -Wab rediraffecheckdiff . _build/html | ||
| } | ||
| build_documentation(){ | ||
| echo_success "Building..." |
| else | ||
| sphinx-build -Wa . _build/html -j `sysctl -n hw.ncpu` || exit $? | ||
| fi | ||
| sphinx-build -Wab rediraffecheckdiff . _build/html |
There was a problem hiding this comment.
Should this also || exit ?
That should also allow you to put a echo_success after
| install_via_pacman | ||
| else | ||
| echo "Unsupported package manager. Please install the packages and create the virtualenv manually." | ||
| echo_error "Unsupported package manager. Please install the packages and create the virtualenv manually." |
There was a problem hiding this comment.
The way it's implemented right now with exit 1, it's misleading to say users should install dependecies, since even if they did, it would still error out. Better to be factual and just say "Failed to detect package manager"
If the exit would be removed, the script could continue. In that case this should be a warning about package manager detection.
In no case does it make sense to instruct about virtualenv, since this would be a separate subsequent step.
|
@grenudi maybe have a look at update_translations.sh script. Looks like as if it started as a copy/paste of an older build script. By now it's diverged a lot and with your change would diverge more. Maybe this could be added here instead with a --translation option or something |
ty, I will look into it |
2 participants
IMPORTANT note:
I removed the ability to pass UBPORTSDOCSENV straight after ./build.sh
like so:
./build.sh ~/ubportsdocsenv/now it's:
./build.sh --env ~/ubportsdocsenv/or for the full output like before it's now:
./build.sh --env ~/ubportsdocsenv/ --verbose