A few more CI workflow comment and style improvements#1672
Merged
Sebastian Thiel (Byron) merged 3 commits intoNov 13, 2024
Conversation
These didn't make it into GitoxideLabs#1668. Besides comments, the changes are for consistency with the prevailing style, usually by omitting redundant YAML quoting. Removal of the outer double quotes in the `if` in `tests-pass` is a case of this, and produces an equivalent node in parsing (i.e. its equivalence does not depend on anything about GHA itself). But just to be sure, I did run yq '.jobs.tests-pass.steps[0].if' .github/workflows/ci.yml before and after the change, to ensure the output was the same. The other change here that deserves comment is the removal of `--` as an argument to a `diff` command. When any path argument is formed from paramter expansion or from a glob with a leading `*` or other globbing character, `--` helps express that the following arguments are not options. For `git diff`, a `--` expresses that the following arguments are neither options nor refs, but paths, so all `git diff` commands with paths in the CI workflows use `--` even if no shell expansions are involved. (In practice this means `--` is often useful for `diff` with paths and, based on this habit, I had inadvertently written a `--` where neither of the above scenarios applied. But that had actually decreased stylistic consistency because we are not using `--` elsewhere that the meaning of all arguments after it is unambiguous even without examining any surrounding context.)
And try omitting libc-dev.
Sebastian Thiel (Byron)
approved these changes
Nov 13, 2024
Sebastian Thiel (Byron)
left a comment
Sebastian Thiel (Byron)
left a comment
Member
There was a problem hiding this comment.
Thanks so much, this is wonderful.
I particularly like the writeup on when to use --, as somehow I also settled in "always trying to use it when available" which may be unnecessary, or reduce readability at no improvement whatsoever.
I also like the shell-array to make comments possible.
Eliah Kagan (EliahKagan)
added a commit
to EliahKagan/gitoxide
that referenced
this pull request
Nov 17, 2024
This uses the same array+comment style as in 8c71bd1 (GitoxideLabs#1672).
This file contains hidden or 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
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
2 participants
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.
These didn't make it into #1668.
Comments are probably the most significant change here.
In the case of
pure-rust-build, I verified that we really do need the C-related packages, then refactored to facilitate the inclusion of specific, readable comments. (This refactoring used an array, whichshis not required to support, and it is running in a container, where GHA defaultsshelltosh, which is why I setbashexplicitly for the step.)Other changes, besides comments, are for consistency with the prevailing style, usually by omitting redundant YAML quoting.
Removal of the outer double quotes in the
ifintests-passis a case of this, and produces an equivalent node in parsing (i.e. its equivalence does not depend on anything about GHA itself). But just to be sure, I did runyq '.jobs.tests-pass.steps[0].if' .github/workflows/ci.ymlbefore and after the change, to ensure the output was the same. Note that omitting the double quotes here has a hidden benefit besides the improvement in consistency and slight improvement in readability: both with and without them are syntactically correct, but the VS Code extension for GitHub Actions wrongly reports the double-quoted syntax as an error here while accepting this syntax.
The other change here that deserves comment is the removal of
--as an argument to adiffcommand. When any path argument is formed from paramter expansion or from a glob with a leading*or other globbing character,--helps express that the following arguments are not options. Forgit diff, a--expresses that the following arguments are neither options nor refs, but paths, so allgit diffcommands with paths in the CI workflows use--even if no shell expansions are involved.(In practice this means
--is often useful fordiffwith paths and, based on this habit, I had inadvertently written a--where neither of the above scenarios applied. But that had actually decreased stylistic consistency because we are not using--elsewhere that the meaning of all arguments after it is unambiguous even without examining any surrounding context.)