WIP - Bash code review #209
Draft
Conversation
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
|
The change itself is fine. Feel welcome to continue adding more in this PR or create an individual PR right away. I don't mind trivial PRs :) |
|
Started out today making a bunch of changes to the Makefile. To be clear, this code review will likely happen in fits and spurts over the next several (7-10?) days, as I have time and ambition. I'll get into the Bash files tomorrow... |
|
Just pushed a bunch of Bash review commits. Kept almost all changes to just I still have lots more to get to, but out of time for today. |
okurz
reviewed
Jan 30, 2023
|
about "Add a run-with Makefile function" I am not sure it's a good idea to "just" warn. I would prefer a fatal failure. |
okurz
reviewed
Jan 30, 2023
_common
Outdated
Comment on lines
70
to
74
| [[ "$rc" != 0 ]] && warn "curl ($(caller)): Error fetching ($*): $response" && return 1 | ||
| [[ "$rc" == 0 ]] || die "curl ($(caller)): Error fetching ($*): $response" | ||
| status_code=$(echo "$response" | tail -1) | ||
| [[ "$status_code" != 200 ]] && warn "curl ($(caller)): Error fetching url ($*): Got Status $status_code" && return 1 | ||
| [[ "$status_code" == 200 ]] || die "curl ($(caller)): Error fetching url ($*): Got Status $status_code" |
There was a problem hiding this comment.
but calling die is having a different effect than just returning from this function. I have not checked all calls of runcurl but if you want to replace the usage here then we should make sure it's effectively having the same effect
There was a problem hiding this comment.
That's a very good point.
exit is not same as return, though with set -e returning non-zero could trigger an exit depending on how it was called.
This inspires me to create a test coverage facility.
okurz
reviewed
Jan 30, 2023
_common
Outdated
| [[ "$rc" == 0 ]] && echo "$output" && return | ||
| [[ $rc -eq 0 ]] && echo "$output" && return |
There was a problem hiding this comment.
I honestly knew about the subtleties of == vs. eq in bash but still prefered == as a more readable variant. You wrote what we should use but if there isn't a more compelling reason I'd prefer to just keep the "=="
There was a problem hiding this comment.
I suppose if shellcheck isn't griping then it's very likely ok.
I hadn't thought about using == for numbers before but I can't yet think of a reason it wouldn't work.
In that case I'd stick with it. I'll let you know if I find any edge cases.
I might start using this as well if it's safe. :)
|
This pull request is now in conflicts. Could you fix it? 🙏 |
|
everyone, #211 was merged but I highly recommend everyone to read all the git commit messages including the ones of already merged commits as @ingydotnet's explanations are really good and helpful for the future as well :) |
See 31022d7 |
aka - Wrap some long lines in README.md This is just a benign first commit so that I can create a pull request for this repository. Per os-autoinst#208 I wish to code review the Bash content and make suggestions on possible improvements. I haven't done this before so I'm not sure of the best way to go about it. I'm thinking to use this pull request, where I will make a commit for each suggestion with a couple of code changes to demonstrate the objective. Then we can can discuss it. I don't expect the PR to be merged, but you might end up cherry-picking some commits in it. Let me know if this approach works for you and I'll start adding commits soon after.
The run-with function checks to make sure that the runner command is installed before running the test. If not installed it issues a warning message. It used to just fail, which causes all of `make test` to fail. Also before there where lines to check if shellcheck and yamllint were installed. They printed a msg but did not stop the commands from attempting to run only to obviously fail for not being installed. There was no previous check for py.test which was always failing for me since I didn't have it installed. Now when something isn't installed that test is just skipped with an explanation message.
When I write any one-off bash that doesn't use a framework like BPAN, the first line I write is a `die` function, because its always and often needed. I've defined the most basic one here. Since warn is now multi-line, die is as well (if you ever need that). The `bpan` CLI defines a minimal one right away here: https://github.com/bpan-org/bpan/blob/main/bin/bpan#L5 Until it can bootstrap to load a full featured one from here: https://github.com/bpan-org/bashplus/blob/main/lib/bashplus/err.bash
Personally I try to keep all of my coding in any language or format under 80 columns, but that's just a personal preference. However, it has allowed me to learn a lot of tricks for splitting lines in Bash. In this commit I show a bunch of them. And hopefully you agree then changes make things more readable. The best thing to know is that you can split a line after a `|`, `&&` or `||` token. Bash knows you aren't finished with that command and so no backslash `\` is required to continue. Next, `$(...)` forms a code block within. You can put any Bash inside and it can be multiline. The inner Bash follows all the same rules as the outer Bash. The long jq coomands are long strings and bash supports strings to be multiline. Since jq doesn't mind ws to have newlines, it works the same. Note I put `|` chars in jq in front rather than at end of lines, because these `|` have no special meaning to Bash here. I defined header in 2 commands instead of one, as an example for splitting a long line with a cleaner look. `$'...'` is not just a way to quote a newline in Bash. It's a full quoting syntax like `"..."` or `'...'`. It works like `'...'` but supports escapes (but not interpolation). Try `echo $'foo\nbar'`.
I think it's great that you use `[[` instead of `[`. The former is keyword syntax while that latter acts as a command. Being syntax allows Bash to be able to treat what's inside specially. The best thing is you almost never need to use quotes inside `[[`. You do need to double quote a variable on the RHS for the `==` and `!=` operators but shellcheck wil catch that. Speaking of spellcheck is great about catching things that are a problem but not good at pointing out unrequired syntax. I don't like quoting things that don't need quotes as it makes people wonder why you quoted them in the first place. Quoting implies a special semantic intent. You should use the == != <= etc for string comparison and eq ne le etc for numbers. Opposite of Perl. Bourne shell came first so good on Perl for switching it! Note: I left in `== 200` because I think of http status codes as numberic strings. I'm honestly not sure how to treat them. Still learning :) The `-n` operator is never needed. `[[ $foo ]]` means same as `[[ -n $foo ]]` and `[[ ! $foo ]]` means same as `[[ -z $foo ]]`. Changed `[[ $dry_run == 1 ]]` to `[[ $dry_run ]]` as I don't think `1` is special here.
If a function has no desirable side-effects (changing directory, setting
global vars) you can define them like:
fn() ( ... )
instead of
fn() ( ...; )
and that means the same as
fn() { ( ... ); }
The nice thing about this is you don't ever need to use `local` because
the subprocess doesn't allow you to change global state.
Note that a one line function declared this way also needs no final
semicolon.
I changed all your functions that didn't have side-effects to use this
form.
I try to use this form unless I need to set state. It ends up leading to
better overall programming decisions.
Note that functions called in a tight loop that need to be fast as
possible should not use this form as subprocesses incur a slight
overhead. Generally this should not be a concern.
I'm pretty sure this is a clean refactoring of that function.
I thought this logic reads a bit clearer, but not that important.
The echo and cat commands are rarely needed to set up stdin for a
process.
Here I use <<< redirection which is cleaner (and likely faster).
As you can see, in Bash redirections can be placed anywhere in a
command, not just at the end.
These all work the same:
foo a b c <i >o
<i foo a b c >o
>o <i foo a b c
foo a <o b >i c
The 'make test' command will now error when dependencies are not installed. For py.test I made it use a virtualenv install of py.test and requests modules. The Makefile looks for a Python 3 binary and errors otherwise. I had python3 installed locally but not py.test. I didn't feel like that should stop me from installing it temporarily on the fly like we do for bpan.
I put all my best practice setup lines in _common. From conversations with @perlpunk I think that the environments your Bash code runs in would all have Bash 4.2+. Bash 4.4 or higher would be something to aim for. Since _common is a Bash library and not an executable the shebang line is not needed, however it's probably there as a syntax highlighter hint since there is no `.bash` extension on this file. A better name might be `_common.bash` or `.common.bash` or `lib/common.bash`. I changed a couple scripts to use `#!/usr/bin/env bash` shebang lines. This is my preference for all my open source Bash that needs to run everywhere. One cool aspect of this is on OSX where `/bin/bash` must be v3.2 you can do: (shopt -s compat44) || die "Bash 5.0+ required. Try 'brew install bash'." Since brew will install to `/usr/local/bin` and that always comes before `/bin` in a standard/sane `PATH`, Mac users can use Bash 5 with scripts without changing /bin/bash. I realize that OSX is not a target env for you, but you could do the same trick to require a newer Bash on Suse Linux which I understand is still at 4.2. Finally by source-ing _common first thing in all your scripts you can skip the `set -e ...` code as _common does it for you.
It turns out that 'make test-shellcheck' was not picking up the test/ files. The `grep` command I added picks up everything the other command did, plus the test/ files. After that shellcheck griped about a lot in the test files. I've fixed the problematic ones and silenced the others.
|
Rebased master and added some more commits. |
The following assignment forms do not need the RHS quoted:
a=word
b=$var
c=$(cmd)
d=word-$var-$(cmd --even="with $args")
I personally strive not to use unnecessary quotes as it implies they
are needed for some purpose.
Note: shellcheck doesn't care about this since it's valid albeit useless
syntax.
Also note that `${foo-}` is idiomatic for `${foo:-''}`. You see it often
in things like:
[[ ${SOME_VAR-} ]] && do-something
where SOME_VAR might be undefined.
|
Ironically 983e105 is what I imagined would be my first review commit as it is a pet peeve. So much cargo-culted bash scripts and stackoverflow posts over-quote... |
While '.' is a nice alias of 'source' to use at the command line, it's
better to stick to 'source' in Bash code. Just my opinion, but it's more
clear to future readers (and easier to grep for).
I remove extra 'set -e' calls for things sourcing _common since it is
handled in there.
I don't really like putting flags like '-e' on the shebang line.
Shebangs are not very portable across platforms.
Use '${BASH_SOURCE[0]}' to get file path rather than '$0'. '$0' is set
to 'bash' if the file is sourced rather than ran. It's useful in testing
to have an executable script also be source-able. You can unit test the
functions that way.
My standard way to call a 'main' function is:
[[ $0 != "${BASH_SOURCE[0]}" ]] || main "$@"
Note using == w/ && here doesn't work the same. Don't do that.
Final note: http://redsymbol.net/articles/unofficial-bash-strict-mode/
makes a terrible suggestion of setting the 'IFS' variable. imho, it
shows a misunderstanding of what IFS provides.
See how it affects "$*" by running:
(IFS=$'\t\n'; set -- a b c; echo "$*")
I just noticed they do give an alternative at the bottom of that page.
The "alternative" is actually the rudimentary usage of double quoting
"$@" which you should always do. Shellcheck will flag if you don't.
PS Setting IFS locally (per command) is very useful for controlling
specific operations, but be careful to never change it globally.
|
This pull request is now in conflicts. Could you fix it? 🙏 |
3 similar comments
|
This pull request is now in conflicts. Could you fix it? 🙏 |
|
This pull request is now in conflicts. Could you fix it? 🙏 |
|
This pull request is now in conflicts. Could you fix it? 🙏 |
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
Per #208 I wish to code review the Bash content and make suggestions on possible improvements.
I haven't done this before so I'm not sure of the best way to go about it.
I'm thinking to use this pull request, where I will make a commit for each suggestion with a couple of code changes to demonstrate the objective.
Then we can can discuss it.
I don't expect the PR to ever be merged, but you might end up cherry-picking some commits in it.
Let me know if this approach works for you and I'll start adding commits soon after.