bin(run-tests): refactor #299
Merged
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 shebang in this file is
#!/usr/bin/env bash
So let's prefer [[.
This also matches the style in the `bin/fetch-configlet` script.
See e.g. the Google Shell Style Guide [1].
[1] https://google.github.io/styleguide/shellguide.html#s6.3-tests
Allow defining more variables as `local`, and work towards matching the style of the bin/fetch-configlet script. This is also suggested in the Google Shell Style Guide [1]. [1] https://google.github.io/styleguide/shellguide.html#main
Signal to the reader that there's nothing complex here.
From the Google Shell Style Guide [1]:
Ensure that local variables are only seen inside a function and its
children by using local when declaring them. This avoids polluting
the global name space and inadvertently setting variables that may
have significance outside the function.
And again, this matches the style in `bin/fetch-configlet`.
[1] https://google.github.io/styleguide/shellguide.html#s7.6-use-local-variables
Be stricter, and improve readability.
We already pass the -f option.
And remove the directory existence check that previously existed
immediately after
for exercise in *; do
Allow moving the `rm` command closer to the `cp` command.
Make it easier to see that the `zig test` command is the same, regardless of where the zig executable is located. The cost: it's no longer possible to move your zig installation while the script is executing. But we don't need to support that.
If a directory is removed in the time between the outer validation/globbing and here, just let `cd` error.
As we do elsewhere in this script.
ErikSchierboom
approved these changes
Aug 22, 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.
Make the
run-testsscript more readable (at least for my taste), and more consistent with the style inbin/fetch-configlet.This is just refactoring - no change in functionality. But we exchange some theoretical TOCTOU subtleties for other ones. See individual commits for more details. A whitespace-ignoring or whitespace-aware diff is more readable here.
Summary:
[[to[for bash. See e.g. this section in the Google Shell Style Guide.local. This is also suggested (here and here) in the same guide, and matchesbin/fetch-configlet.-fand-dto-e.printfmore consistently.