Cherry-pick all bash code review commits I consider fine right away #211
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
Using test-tap-bash for testing was made simple by having 'make test' clone the bpan-org/bpan repo which bundles test-tap-bash and other core bpan libraries. The bpan directory is gitignored but deserves to have a 'make clean' to set the repo back to a pristine state. FWIW, as much as I am a fan of Bash best practices I also have been a fanatic of Makefile best usage. I even created https://makeplus.net/ to make make (npi) more powerful. That said, I don't use `.PHONY:` for non file based targets. In this repo the only warranted one is `.PHONY: test` since there is a directory called test. But even that one is not needed since the test target has dependencies `checkstyle test-unit` that will never be satisfied when `make test` is run.
As explained in previous commit, the .PHONY lines serve no purpose except documentation. But it's fairly obvious which targets are phony here. If the `test:` target had no phony dependencies then a `.PHONY: test` would be needed since there is a `test` directory here.
and move internal rules to end of Makefile. Added python generateed files and directories to 'make clean'.
make test test=test/01*.t
and turn on verbose:
make test v=1
Also the xt/ test failed when there were uncommitted changes to the
repo. Made it not run those unless all changes are committed.
Put the check in an include file for various functions.
By default GNU make uses /bin/sh. Since we are explicitly using Bash in this repo it makes a lot of things more reliable to set `SHELL` to `bash`.
Only the test-bash rule depends on BPAN.
One of the benefits here of setting `SHELL := bash` is that we no longer need to use env to set environment variables before a shell command. Bash just handles that itself. Removed a useless use of `cat`. Dropped the `-e` from `bash -ex` as that is handled in ./openqa-label-known-issues-multi itsef. I'm not completely sure about that `grep -v` but it seem like it was trying to avoid the error from invalid JSON (noted in the comment). Putting `-` prefix on a rule command tells `make` not to stop if the command fails. Not sure that is correct here or not.
I ran 'make test-online'. It failed but left that file. Now 'make clean' removes it.
I think it's great that you have an _common and that you define a `warn` function. I changed `echo "$@"` in `warn`. First off it would have been more aapropriate to have used `"$*"` if you wanted to join all args into one string. Understanding how `"$@"` vs `"$*"` works is fundamental to bash programming. `"$@"` expands to a list, not a string. The double quotes tell it to not word split on values with whitespace in them. `"$*"` is what you use to join `$@` elements into a string. `printf '%s\n' "$@"` will print each argument on its own line. This is how I defined `warn` all the time. Handy if you need a warning with more than one line. I then used warn instead of echo throughout the file. NOTE: Looking over _common, it is rich with things to improve. I think I'll stick to just making commits on this file for now.
I know you already know this, but you can use `-` in function names. In fact you can use just about any character that you could use in a file name. I often use ':' to namespace for a module. Like this: `json:dump-to-file` I find that avoiding underscores in function names leads to easier to read code. Variables use _ and functions don't.
baierjan
approved these changes
Jan 30, 2023
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.
I cherry-picked the following commits from #209
that I consider right away to merge if others agree as well. Other not included commits can still be discussed:
warnin _commonenvfrom Makefile rulestest-unitMakefile rule into 2 rules