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

A few more CI workflow comment and style improvements #1672

Merged
merged 3 commits into from
Nov 13, 2024

Conversation

EliahKagan
Copy link
Member

@EliahKagan EliahKagan commented Nov 12, 2024

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, which sh is not required to support, and it is running in a container, where GHA defaults shell to sh, which is why I set bash explicitly 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 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. 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 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.)

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.
Copy link
Member

@Byron Byron left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

@Byron Byron merged commit eedebe7 into GitoxideLabs:main Nov 13, 2024
18 checks passed
@EliahKagan EliahKagan deleted the workflow-style branch November 13, 2024 06:27
EliahKagan added a commit to EliahKagan/gitoxide that referenced this pull request Nov 17, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants