Develop #154
Develop #154
Conversation
WalkthroughUpdates CI workflow to restructure build/publish matrix, standardize binary naming via an environment variable, integrate packaging and release steps, and update Homebrew automation. In the gitignore command, adds normalization to handle names with a trailing “.gitignore”. Adds integration tests validating add and preview behaviors with normalized names. Changes
Sequence Diagram(s)sequenceDiagram
autonumber
participant U as User
participant CLI as gh-templates (gitignore)
participant N as Normalizer
participant C as Template Cache
participant FS as File System
U->>CLI: gitignore add rust.gitignore / preview rust.gitignore
CLI->>N: Normalize name
Note right of N: Strip trailing ".gitignore" if present
N-->>CLI: rust
CLI->>C: Lookup "rust" template
alt found in cache
C-->>CLI: Template content
else not in cache
CLI->>C: Update/fetch into cache (if requested)
C-->>CLI: Template content or error
end
opt add flow
CLI->>FS: Write to .gitignore (path unchanged)
FS-->>CLI: OK
end
CLI-->>U: Output preview or success message
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~20 minutes Possibly related PRs
Poem
Pre-merge checks and finishing touches❌ Failed checks (2 warnings, 1 inconclusive)
✨ Finishing touches
🧪 Generate unit tests (beta)
Comment |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 4
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
.github/workflows/build.yml (1)
46-49: Update deprecated action to fix runner compatibility.The
actions-rs/toolchain@v1action uses an outdated runner that may not be compatible with current GitHub Actions infrastructure.Apply this diff to use the recommended
dtolnay/rust-toolchainaction:- - name: Setup Rust - uses: actions-rs/toolchain@v1 - with: - toolchain: stable - target: ${{ matrix.build.TARGET }} + - name: Setup Rust + uses: dtolnay/rust-toolchain@stable + with: + targets: ${{ matrix.build.TARGET }}
🧹 Nitpick comments (1)
.github/workflows/build.yml (1)
182-186: Fix hardcoded binary names in Homebrew formula to match new naming convention.The
installsection references hardcoded binary names (e.g.,gh-templates-darwin-arm64) that match the new naming convention, but the formula doesn't account for potential future changes to the matrix build names.The current implementation is correct and matches the binary names generated in the workflow. However, if the matrix build
NAMEvalues change, the formula will need to be updated accordingly. Consider documenting this dependency or adding a comment in the workflow to remind maintainers to update the formula if build names change.No immediate code change is required, but this is something to keep in mind for future maintenance.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (4)
.github/workflows/build.yml(5 hunks)src/commands/gitignore/add.rs(1 hunks)src/commands/gitignore/preview.rs(1 hunks)tests/integration/gitignore_tests.rs(4 hunks)
🧰 Additional context used
🧬 Code graph analysis (1)
tests/integration/gitignore_tests.rs (1)
tests/common/test_utils.rs (4)
setup_test_env(4-7)create_git_repo(9-15)assert_file_exists(22-24)assert_file_contains(17-20)
🪛 actionlint (1.7.7)
.github/workflows/build.yml
20-20: the runner of "actions-rs/toolchain@v1" action is too old to run on GitHub Actions. update the action's version to fix this issue
(action)
119-119: input "overwrite" is not defined in action "softprops/action-gh-release@v2". available inputs are "append_body", "body", "body_path", "discussion_category_name", "draft", "fail_on_unmatched_files", "files", "generate_release_notes", "make_latest", "name", "prerelease", "preserve_order", "repository", "tag_name", "target_commitish", "token"
(action)
🔇 Additional comments (3)
src/commands/gitignore/preview.rs (1)
39-41: LGTM! Normalization logic is correct.The normalization step correctly handles template names with or without the ".gitignore" suffix, ensuring consistent lookup behavior. The use of
strip_suffix().unwrap_or()is idiomatic and safe.tests/integration/gitignore_tests.rs (2)
63-81: LGTM! Test coverage for normalization is appropriate.The new test correctly verifies that adding a template with the ".gitignore" suffix works as expected, exercising the normalization logic added in
add.rs.
423-434: LGTM! Integration test is well-structured.The test validates the preview command with cache update, ensuring the normalization and caching logic work correctly together.
| uses: actions-rs/toolchain@v1 | ||
| with: | ||
| toolchain: stable |
There was a problem hiding this comment.
🛠️ Refactor suggestion | 🟠 Major
Update deprecated action to fix runner compatibility.
The actions-rs/toolchain@v1 action uses an outdated runner that may not be compatible with current GitHub Actions infrastructure.
Apply this diff to use the recommended dtolnay/rust-toolchain action:
- - name: Setup Rust
- uses: actions-rs/toolchain@v1
- with:
- toolchain: stable
+ - name: Setup Rust
+ uses: dtolnay/rust-toolchain@stable📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| uses: actions-rs/toolchain@v1 | |
| with: | |
| toolchain: stable | |
| - name: Setup Rust | |
| uses: dtolnay/rust-toolchain@stable |
🧰 Tools
🪛 actionlint (1.7.7)
20-20: the runner of "actions-rs/toolchain@v1" action is too old to run on GitHub Actions. update the action's version to fix this issue
(action)
🤖 Prompt for AI Agents
.github/workflows/build.yml lines 20-22: the workflow uses the deprecated
actions-rs/toolchain@v1 action; replace it with the recommended
dtolnay/rust-toolchain action to ensure runner compatibility by updating the
uses entry to dtolnay/rust-toolchain@v1 and keep the toolchain input set to
"stable" (i.e., change the action reference while preserving the toolchain:
stable input).
| cat > python/gh_templates_bin/__init__.py << 'EOF' | ||
| import os, sys, subprocess | ||
| def main(): | ||
| binary = os.path.join(os.path.dirname(__file__), '$BINARY') | ||
| binary = os.path.join(os.path.dirname(__file__), os.getenv("BINARY", "gh-templates")) | ||
| sys.exit(subprocess.run([binary] + sys.argv[1:]).returncode) | ||
| EOF |
There was a problem hiding this comment.
Fix incorrect environment variable reference in Python wrapper.
The Python __init__.py references os.getenv("BINARY", ...), but the BINARY environment variable is only available during the GitHub Actions build workflow, not when end users run the installed Python package. This will cause the wrapper to fail at runtime.
Apply this diff to use the actual binary filename directly:
# Python __init__.py
cat > python/gh_templates_bin/__init__.py << 'EOF'
import os, sys, subprocess
def main():
- binary = os.path.join(os.path.dirname(__file__), os.getenv("BINARY", "gh-templates"))
+ binary = os.path.join(os.path.dirname(__file__), "$BINARY")
sys.exit(subprocess.run([binary] + sys.argv[1:]).returncode)
EOFNote: The $BINARY will be expanded by the shell since the heredoc delimiter EOF is unquoted. Alternatively, use a more explicit approach by writing the file with the expanded variable.
🤖 Prompt for AI Agents
.github/workflows/build.yml around lines 108-113: the generated
python/gh_templates_bin/__init__.py uses os.getenv("BINARY", ...) which looks
for BINARY at runtime and will be missing for users; instead ensure the build
writes the actual binary filename into the file by expanding the shell variable
at template-write time (or hardcode the intended binary name). Replace the
heredoc content so the generated file uses the concrete binary path (e.g.,
join(dirname, "$BINARY" expanded by the shell) or the literal binary name)
rather than os.getenv, so the wrapper calls the correct executable for installed
packages.
.github/workflows/build.yml
Outdated
| - name: Upload to GitHub Release | ||
| uses: softprops/action-gh-release@v2 | ||
| with: | ||
| files: gh-templates* | ||
| files: ${{ env.BINARY }} | ||
| overwrite: true | ||
| fail_on_unmatched_files: false |
There was a problem hiding this comment.
Remove invalid input parameter.
The overwrite input is not supported by softprops/action-gh-release@v2. The action handles overwriting automatically when files with the same name exist.
Apply this diff to remove the invalid parameter:
- name: Upload to GitHub Release
uses: softprops/action-gh-release@v2
with:
files: ${{ env.BINARY }}
- overwrite: true
fail_on_unmatched_files: false📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| - name: Upload to GitHub Release | |
| uses: softprops/action-gh-release@v2 | |
| with: | |
| files: gh-templates* | |
| files: ${{ env.BINARY }} | |
| overwrite: true | |
| fail_on_unmatched_files: false | |
| - name: Upload to GitHub Release | |
| uses: softprops/action-gh-release@v2 | |
| with: | |
| files: ${{ env.BINARY }} | |
| fail_on_unmatched_files: false |
🧰 Tools
🪛 actionlint (1.7.7)
119-119: input "overwrite" is not defined in action "softprops/action-gh-release@v2". available inputs are "append_body", "body", "body_path", "discussion_category_name", "draft", "fail_on_unmatched_files", "files", "generate_release_notes", "make_latest", "name", "prerelease", "preserve_order", "repository", "tag_name", "target_commitish", "token"
(action)
🤖 Prompt for AI Agents
.github/workflows/build.yml around lines 115 to 120: the workflow uses
softprops/action-gh-release@v2 and includes an unsupported input "overwrite:
true"; remove the invalid "overwrite: true" line from the action inputs (leave
the files and fail_on_unmatched_files inputs intact) so the action relies on its
built-in overwrite behavior.
| // check if the template_name has a .gitignore ext rm it to normalize it | ||
| let template_name = template_name.strip_suffix(".gitignore").unwrap_or(template_name); |
There was a problem hiding this comment.
🧩 Analysis chain
Verify normalization consistency across all code paths.
The normalization logic is correct. However, ensure that normalization is consistently applied in all branches where find_template_in_cache is called.
Run the following script to verify that find_template_in_cache calls handle normalization consistently:
🏁 Script executed:
#!/bin/bash
# Description: Check all usages of find_template_in_cache to ensure normalization is applied consistently
# Find all calls to find_template_in_cache
rg -n -A 3 -B 3 'find_template_in_cache' --type rustLength of output: 3636
🏁 Script executed:
#!/bin/bash
# Show find_template_in_cache implementation with context
rg -nP 'fn find_template_in_cache' -C20 --type rust src/commands/gitignore/mod.rsLength of output: 1346
🏁 Script executed:
#!/bin/bash
# Show AddArgs definition and how templates are parsed
rg -nP 'pub struct AddArgs' -C20 --type rust src/commands/gitignore/add.rsLength of output: 1005
Normalize template names by stripping the “.gitignore” suffix before cache lookup in all branches.
- In
src/commands/gitignore/add.rs, use_remote_name branch (around line 178): apply
let name = template_name.strip_suffix(".gitignore").unwrap_or(template_name);
before callingfind_template_in_cache(name, cache). - In the fallback branch (around line 248): do the same normalization on
template_namebefore lookup.
🤖 Prompt for AI Agents
In src/commands/gitignore/add.rs around lines 178 to 248, the template lookup
sometimes uses template_name with a trailing ".gitignore" causing cache misses;
normalize the template name by calling
template_name.strip_suffix(".gitignore").unwrap_or(template_name) (assign to a
local variable like name) before passing it to find_template_in_cache in the
use_remote_name branch (~line 178) and again in the fallback branch (~line 248)
so both branches perform cache lookup on the normalized name.
…ectly.\n\nInitially we used to allow only the template name without the the ext though some users may pass the '.gitignore' ext. It shouldn't fail but be parsed correctly\n Signed-off-by: rafaeljohn9 <rafaeljohb@gmail.com>
…tes with .gitignore is succesful. Signed-off-by: rafaeljohn9 <rafaeljohb@gmail.com>
…ion of the gitignore template name with ext Signed-off-by: rafaeljohn9 <rafaeljohb@gmail.com>
…tions - Append matrix build name to binary filenames (e.g., gh-templates-linux-x64-glibc) - Update release upload step to use renamed binaries - Prevent duplicate asset deletion errors with overwrite & fail_on_unmatched_files - Keep Homebrew formula aligned with new asset names
develop => main
Summary by CodeRabbit