⚠ This page is served via a proxy. Original site: https://github.com
This service does not collect credentials or authentication data.
Skip to content

Conversation

@jessealama
Copy link
Contributor

@jessealama jessealama commented Dec 6, 2025

Fixes #202

Addresses all shellcheck warnings in scripts/create-adaptation-pr.sh:

  • Quote variables to prevent globbing/word splitting (SC2086)
  • Use $(...) instead of legacy backticks (SC2006)
  • Add -r flag to read commands (SC2162)

@jessealama jessealama requested a review from kim-em as a code owner December 6, 2025 21:41
@fmontesi
Copy link
Collaborator

LGTM but I'll wait for @kim-em as well, since this might be interesting in general for other repos, too.

@arademaker
Copy link
Collaborator

@kim-em , can we accept this PR? It looks good to me.

@kim-em
Copy link
Collaborator

kim-em commented Jan 23, 2026

@kim-em
Copy link
Collaborator

kim-em commented Jan 23, 2026

Minor suggestion: the quoting style could be more consistent. The PR uses bump/"$BUMPVERSION" but the existing code already uses "bump/$BUMPVERSION" in other places (e.g., line 94).

For consistency, consider using:

# Instead of:
git checkout bump/"$BUMPVERSION" -- lean-toolchain
# Use:
git checkout "bump/$BUMPVERSION" -- lean-toolchain

Both work correctly, but quoting the whole string is more idiomatic and matches the existing style in the script.

I've updated the corresponding batteries and mathlib4 PRs to use the consistent style:

Copy link
Collaborator

@kim-em kim-em left a comment

Choose a reason for hiding this comment

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

Looks good to me, thanks for looking into this.

@chenson2018
Copy link
Collaborator

(I merged main so CI would let me put this on the merge queue)

@chenson2018 chenson2018 enabled auto-merge January 23, 2026 20:40
@chenson2018 chenson2018 added this pull request to the merge queue Jan 23, 2026
Merged via the queue into leanprover:main with commit cff6f95 Jan 23, 2026
2 checks passed
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.

shellcheck warnings in some bash scripts

5 participants