Conversation
|
Hi @3rly , looks good to me but then again I don't know anything about Bash so my "looks good to me" really doesn't mean much! @AlexanderWillner , do you see any problems with merging this PR? |
|
The change 1df7a35 seems wrong to me:
|
|
When I get home, I’ll investigate the why that function call it isn’t working manually for you (it passed all my tests on locally FreeBSD, Arch and a Debian-based VMs). Moreover, that command is un-modified from the original script. My additions were simply the case/switch statements. |
|
Just to clarify: I just tested the Linux and BSD commands on MacOS. As say seem not to work, the switch statement makes sense. |
|
@AlexanderWillner If you are using the coreutils package from Homebrew, there will be a conflict per the package maintainer's formula: source lines[44:80] respectively. Commands also provided by macOS have been installed with the prefix "g".
If you need to use these commands with their normal names, you
can add a "gnubin" directory to your PATH from your bashrc like:
PATH="$(brew --prefix)/opt/coreutils/libexec/gnubin:$PATH" |
|
I think |
| function moveChaptersToSrcDir(){ | ||
| for f in Chapter_*.md; do | ||
| mv $f src/$f | ||
| mv "$f" src/"$f" |
| mdBook build && mdBook serve --open | ||
| mdbook build && mdbook serve --open |
| # Check for binaries | ||
| function checkEnvironment(){ | ||
| type gcsplit >/dev/null 2>&1 || { echo "Install 'gcsplit' first (e.g. via 'brew install coreutils')." >&2 && exit 1 ; } | ||
| type csplit >/dev/null 2>&1 || { echo "Install gcsplit' first (e.g. via 'brew install coreutils')." >&2 && exit 1 ; } |
There was a problem hiding this comment.
I wouldn't remove the gcsplit line.
Because on macOS gcsplit is used and it cannot be caught in Environment.
What you want to do is check for os type here using an if-else and then execute the check line for that os type.
| # Splits the Readme.md file based on the header in markdown and creates chapters | ||
| # Note: | ||
| # Get gcsplit via homebrew on mac: brew install coreutils | ||
| # Get csplit via homebrew on mac: brew install coreutils |
There was a problem hiding this comment.
This is again wrong. On macOS you are using the gcsplit still on line 32 in this file. So saying csplit is being installed here is wrong.
You might want to write this:
# For macOS: Get gcsplit via homebrew, cmd: brew install coreutils
# For Linux: Get csplit via distro's package manager
| OS_NAME=$(uname | tr '[:upper:]' '[:lower:]') | ||
|
|
||
| case $OS_NAME in | ||
| linux*) | ||
| OS_NAME='linux' | ||
| csplit --prefix='Chapter_' --suffix-format='%d.md' --elide-empty-files README.md '/^## /' '{*}' -q | ||
| ;; | ||
| darwin*) | ||
| OS_NAME='osx' | ||
| gcsplit --prefix='Chapter_' --suffix-format='%d.md' --elide-empty-files README.md '/^## /' '{*}' -q | ||
| ;; | ||
| freebsd*) | ||
| OS_NAME='freebsd' | ||
| csplit --prefix='Chapter_' --suffix-format='%d.md' --elide-empty-files README.md '/^## /' '{*}' -q | ||
| ;; | ||
| *) | ||
| OS_NAME=notset | ||
| ;; | ||
| esac |
There was a problem hiding this comment.
I personally donot prefer the switch statements in bash scripts. They are hard to read compared to simpler if-elif statements.
You could have done something like below and executed the specific command:
if [[ "$OSTYPE" == "linux-gnu" ]]; then
# ...
elif [[ "$OSTYPE" == "darwin"* ]]; then
# Mac OSX
elif [[ "$OSTYPE" == "cygwin" ]]; then
# POSIX compatibility layer and Linux environment emulation for Windows
elif [[ "$OSTYPE" == "msys" ]]; then
# Lightweight shell and GNU utilities compiled for Windows (part of MinGW)
elif [[ "$OSTYPE" == "freebsd"* ]]; then
#FreeBSD
else
# Unknown.
fi
4 participants
I made this change originally just to get it working on my machine. However, this refactored version could be beneficial to both GNU/Linux and OSX users.
** Didn't have time tonight, but if you prefer, I can also figure out WSL support.
Thank you for the book. I am enjoying it greatly.