Conversation
WalkthroughUpdates a shell script to enforce unset-variable errors, parameterizes the Unicode data download URL using $UNICODE_VERSION, and retains the existing ucd-generate invocation. No exported/public interfaces are changed. Changes
Estimated code review effort🎯 2 (Simple) | ⏱️ ~10 minutes Suggested reviewers
Pre-merge checks (3 passed)✅ Passed checks (3 passed)
Poem
Tip 👮 Agentic pre-merge checks are now available in preview!Pro plan users can now enable pre-merge checks in their settings to enforce checklists before merging PRs.
Please see the documentation for more information. Example: reviews:
pre_merge_checks:
custom_checks:
- name: "Undocumented Breaking Changes"
mode: "warning"
instructions: |
Pass/fail criteria: All breaking changes to public APIs, CLI flags, environment variables, configuration keys, database schemas, or HTTP/GraphQL endpoints must be documented in the "Breaking Change" section of the PR description and in CHANGELOG.md. Exclude purely internal or private changes (e.g., code not exported from package entry points or explicitly marked as internal).Please share your feedback with us on this Discord post. ✨ Finishing Touches
🧪 Generate unit tests
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
There was a problem hiding this comment.
Actionable comments posted: 2
🧹 Nitpick comments (1)
update_unicode.sh (1)
13-15: Reproducibility: pinucd-generateand avoid masking install failures.The earlier
cargo install ucd-generate || true(Line 4) masks failures and may use an arbitrary preinstalled version, risking non-reproducible outputs for these generated files. Pin and fail on install; also log the version before generation.Suggested (outside the selected lines):
if ! command -v ucd-generate >/dev/null; then cargo install --locked ucd-generate@<known-good-version> fi ucd-generate --versionIf you want, I can propose a full patched script with
set -euo pipefail,pushd/popd, robust curl, and pinneducd-generate.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (2)
pest/src/unicode/script.rs(21 hunks)update_unicode.sh(1 hunks)
🔥 Files not summarized due to errors (1)
- pest/src/unicode/script.rs: Error: Server error: no LLM provider could handle the message
👮 Files not reviewed due to content moderation or server errors (1)
- pest/src/unicode/script.rs
🧰 Additional context used
🪛 Shellcheck (0.10.0)
update_unicode.sh
[warning] 8-8: Use 'cd ... || exit' or 'cd ... || return' in case cd fails.
(SC2164)
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (8)
- GitHub Check: check for semver compatibility
- GitHub Check: check for no_std compatibility
- GitHub Check: cargo hack check --feature-powerset
- GitHub Check: Test Coverage
- GitHub Check: Minimal Versions Testing
- GitHub Check: Documentation check
- GitHub Check: Unit, Style, and Lint Testing
- GitHub Check: Fuzzing
| @@ -1,14 +1,15 @@ | |||
| #!/usr/bin/env bash | |||
| set -u | |||
There was a problem hiding this comment.
🛠️ Refactor suggestion
Fail fast: prefer set -euo pipefail.
Only set -u is enabled. If curl/unzip fails (e.g., 404), the script will continue and later steps may silently generate stale files. Enable -e and -o pipefail to stop on errors.
Apply:
-set -u
+set -euo pipefailAlso consider cd guards per ShellCheck SC2164 (use pushd/popd or cd ... || exit 1).
🤖 Prompt for AI Agents
In update_unicode.sh around line 2, the script currently only uses "set -u"
which won’t stop execution on command failures or failed pipes; change the shell
options to "set -euo pipefail" to fail fast on errors and pipe failures, and
ensure any directory changes use guarded commands (e.g., use pushd/popd or "cd
<dir> || exit 1") so the script exits immediately if a cd fails.
| curl -LO https://www.unicode.org/Public/$UNICODE_VERSION/ucd/UCD.zip | ||
| unzip UCD.zip |
There was a problem hiding this comment.
🛠️ Refactor suggestion
Harden the download and validate UNICODE_VERSION.
- Add a friendly check for
UNICODE_VERSION(currentset -uwill emit a generic error). - Make
curlfail on HTTP errors and surface logs; quote the URL; write to a known filename. - Minor: switch to
rm -rf+mkdir -pandpushd/popdfor idempotence and safer directory handling.
Minimal changes:
-curl -LO https://www.unicode.org/Public/$UNICODE_VERSION/ucd/UCD.zip
+ : "${UNICODE_VERSION:?Set UNICODE_VERSION (e.g., 17.0.0)}"
+curl -fSLo UCD.zip "https://www.unicode.org/Public/${UNICODE_VERSION}/ucd/UCD.zip"Nice-to-haves (outside the selected lines):
rm -rf ./target/ucd/
mkdir -p ./target/ucd/
pushd ./target/ucd/ >/dev/null || exit 1
# ... curl/unzip ...
popd >/dev/null || exit 1Optionally verify integrity if upstream provides checksums:
# Example if a checksum file exists alongside UCD.zip
curl -fSLo UCD.zip.md5 "https://www.unicode.org/Public/${UNICODE_VERSION}/ucd/UCD.zip.md5"
md5sum -c UCD.zip.md5🤖 Prompt for AI Agents
In update_unicode.sh around lines 9-10, the script should validate
UNICODE_VERSION with a friendly error if unset, create/clean a target directory
(rm -rf then mkdir -p), change into it with pushd (and return with popd) for
idempotence, download the UCD zip to a known filename using curl with flags to
fail on HTTP errors and show errors (e.g. -fSLo) while quoting the URL built
from ${UNICODE_VERSION}, and then unzip that known filename; update the script
to implement these steps so downloads fail loudly, filenames are deterministic,
and directory handling is safe.
Summary by CodeRabbit