Skip to content
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

WIP - Bash code review #209

Draft
wants to merge 15 commits into
base: master
Choose a base branch
from
Draft

Commits on Jan 30, 2023

  1. WIP - Bash code review - first commit

    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.
    ingydotnet committed Jan 30, 2023
    Configuration menu
    Copy the full SHA
    52e69a3 View commit details
    Browse the repository at this point in the history
  2. Add a run-with Makefile function

    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.
    ingydotnet committed Jan 30, 2023
    Configuration menu
    Copy the full SHA
    2154a43 View commit details
    Browse the repository at this point in the history
  3. Define and use a die function

    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
    ingydotnet committed Jan 30, 2023
    Configuration menu
    Copy the full SHA
    347ada5 View commit details
    Browse the repository at this point in the history
  4. Various ways to split long lines in 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'`.
    ingydotnet committed Jan 30, 2023
    Configuration menu
    Copy the full SHA
    702badc View commit details
    Browse the repository at this point in the history
  5. Better use of [[ ... ]]

    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.
    ingydotnet committed Jan 30, 2023
    Configuration menu
    Copy the full SHA
    66cff34 View commit details
    Browse the repository at this point in the history
  6. Use the x() (...) function form

    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.
    ingydotnet committed Jan 30, 2023
    Configuration menu
    Copy the full SHA
    6afe96e View commit details
    Browse the repository at this point in the history
  7. Simplify logic in a function

    I'm pretty sure this is a clean refactoring of that function.
    ingydotnet committed Jan 30, 2023
    Configuration menu
    Copy the full SHA
    2b321d8 View commit details
    Browse the repository at this point in the history
  8. Refactor a couple functions in _common

    I thought this logic reads a bit clearer, but not that important.
    ingydotnet committed Jan 30, 2023
    Configuration menu
    Copy the full SHA
    4eda8b3 View commit details
    Browse the repository at this point in the history
  9. Avoid echo ... | ...

    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
    ingydotnet committed Jan 30, 2023
    Configuration menu
    Copy the full SHA
    3ffb566 View commit details
    Browse the repository at this point in the history
  10. Change run-with function to error instead of warn

    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.
    ingydotnet committed Jan 30, 2023
    Configuration menu
    Copy the full SHA
    0df634b View commit details
    Browse the repository at this point in the history
  11. Configuration menu
    Copy the full SHA
    758b8a2 View commit details
    Browse the repository at this point in the history
  12. Changes to Bash bootstrapping code

    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.
    ingydotnet committed Jan 30, 2023
    Configuration menu
    Copy the full SHA
    c04beaa View commit details
    Browse the repository at this point in the history
  13. Fix shellcheck for Bash test files

    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.
    ingydotnet committed Jan 30, 2023
    Configuration menu
    Copy the full SHA
    ffacfa1 View commit details
    Browse the repository at this point in the history
  14. Remove useless quotes on assignments

    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.
    ingydotnet committed Jan 30, 2023
    Configuration menu
    Copy the full SHA
    983e105 View commit details
    Browse the repository at this point in the history

Commits on Jan 31, 2023

  1. Use 'source' instead of '.'

    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.
    ingydotnet committed Jan 31, 2023
    Configuration menu
    Copy the full SHA
    4a01a16 View commit details
    Browse the repository at this point in the history