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

Miscellaneous tiny shell script tweaks #7080

Open
wants to merge 6 commits into
base: main
Choose a base branch
from

Conversation

mhucka
Copy link
Contributor

@mhucka mhucka commented Feb 23, 2025

Running shellcheck raised a couple of warnings in some scripts; this commit addresses them, and also addresses a few other niggles here and there.

  • Handle a couple of places where the set -e at the top of a script would be ignored inside $(...) constructs
  • Change the use of typeset to declare, because the former is considered obsolete
  • Try to declare all variables
  • Use [[ ... ]] consistently instead of a mix of [...] and [[...]]

Using the added `.shellcheckrc` options of PR quantumlib#7079, the `shellcheck`
program flagged a couple of places where the effects of the `set -e`
at the beginning of this script would not have the expected effect.
Ignore a few more extensions when creating a list of candidate files
to test for being shell scripts.
The `typeset` command is considered obsolete; `declare` is its
replacement. While this is not specifically related to the problem of
the "ambiguous redirect", it helps to make sure the use of `typeset`
wasn't a factor.
Some uppercase VARIABLE names in this file are inconsistent in style
with what's done in other files.
@CirqBot CirqBot added the size: M 50< lines changed <250 label Feb 23, 2025
@mhucka mhucka force-pushed the mh-tiny-shell-script-tweaks branch from 7deff0d to c6246ac Compare February 23, 2025 05:11
Copy link

codecov bot commented Feb 23, 2025

Codecov Report

All modified and coverable lines are covered by tests ✅

Project coverage is 98.18%. Comparing base (a2bf6e8) to head (508720c).

Additional details and impacted files
@@           Coverage Diff           @@
##             main    #7080   +/-   ##
=======================================
  Coverage   98.18%   98.18%           
=======================================
  Files        1089     1089           
  Lines       95237    95237           
=======================================
  Hits        93508    93508           
  Misses       1729     1729           

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.

This adds declarations to more variables used in the shell scripts.
Some script used a mix of `[...]` and `[[...]]` for no apparent
reason. It would be better for maintainability to consistently use one
or the other. Since the scripts all explicitly use Bash, we may as
well use `[[...]]` throughout.
@mhucka mhucka force-pushed the mh-tiny-shell-script-tweaks branch from c6246ac to 508720c Compare February 23, 2025 05:35
@mhucka mhucka marked this pull request as ready for review February 23, 2025 05:56
@mhucka mhucka requested review from vtomole and a team as code owners February 23, 2025 05:56
@mhucka mhucka enabled auto-merge February 23, 2025 05:56
@mhucka mhucka self-assigned this Feb 23, 2025
@mhucka mhucka added the kind/health For CI/testing/release process/refactoring/technical debt items label Feb 23, 2025
@mhucka mhucka requested a review from pavoljuhas February 23, 2025 05:57
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
kind/health For CI/testing/release process/refactoring/technical debt items size: M 50< lines changed <250
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants