Add SKIP=true installer mode to preserve existing files#893
Add SKIP=true installer mode to preserve existing files#893
Conversation
WalkthroughAdds a SKIP=true installer mode that causes file operations to skip existing files instead of overwriting; README and CHANGELOG updated. Installer code now selects { force: true }, { skip: true }, or {} based on ENV when invoking Thor file helpers. Changes
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~20 minutes Poem
🚥 Pre-merge checks | ✅ 5 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (5 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing touches
🧪 Generate unit tests (beta)
⚔️ Resolve merge conflicts (beta)
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 |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
Greptile SummaryAdds
Confidence Score: 4/5
Important Files Changed
Flowchartflowchart TD
A[shakapacker:install] --> B{ENV FORCE set?}
B -->|Yes| C["force_option = { force: true }"]
B -->|No| D{ENV SKIP set?}
D -->|Yes| E["force_option = { skip: true }"]
D -->|No| F["force_option = {}"]
C --> G[copy_file / directory]
E --> G
F --> G
G -->|force: true| H[Overwrite existing files]
G -->|skip: true| I[Keep existing, create missing]
G -->|empty| J[Prompt user on conflict]
Last reviewed commit: 9c86f17 |
![greptile-apps[bot]](https://avatars.githubusercontent.com/in/867647?s=60&v=4){kind=small}
lib/install/template.rb
Outdated
| force_option = if ENV["FORCE"] | ||
| { force: true } | ||
| elsif ENV["SKIP"] | ||
| { skip: true } | ||
| else | ||
| {} | ||
| end |
There was a problem hiding this comment.
Variable name is now misleading
The variable force_option can now hold { skip: true }, making the name misleading. Consider renaming to something more general like conflict_option or file_conflict_option to reflect that it handles both force and skip behaviors.
| force_option = if ENV["FORCE"] | |
| { force: true } | |
| elsif ENV["SKIP"] | |
| { skip: true } | |
| else | |
| {} | |
| end | |
| conflict_option = if ENV["FORCE"] | |
| { force: true } | |
| elsif ENV["SKIP"] | |
| { skip: true } | |
| else | |
| {} | |
| end |
Note: This would also require updating the three references to force_option on lines 40, 62, and the corresponding variable in lib/install/binstubs.rb (line 10).
Context Used: Context from dashboard - CLAUDE.md (source)
Note: If this suggestion doesn't match your team's coding style, reply to this and let me know. I'll remember it for next time!
README.md
Outdated
| ``` | ||
|
|
||
| Before initiating the installation process, ensure you have committed all the changes. While installing Shakapacker, there might be some conflict between the existing file content and what Shakapacker tries to copy. You can either approve all the prompts for overriding these files or use the `FORCE=true` environment variable before the installation command to force the override without any prompt. | ||
| Before initiating the installation process, ensure you have committed all the changes. While installing Shakapacker, there might be some conflict between the existing file content and what Shakapacker tries to copy. You can either approve all the prompts for overriding these files, use `FORCE=true` to overwrite without prompting, or use `SKIP=true` to preserve existing files and only create missing ones. |
There was a problem hiding this comment.
Consider documenting FORCE+SKIP precedence
The PR description states that FORCE wins when both are set, which is a useful detail for users. Consider adding this to the documentation, e.g.: "If both are set, FORCE=true takes precedence."
Note: If this suggestion doesn't match your team's coding style, reply to this and let me know. I'll remember it for next time!
justin808
force-pushed
the
codex/issue-889-skip-installer
branch
from
9c86f17 to
a9509c2
Compare
|
@claude review this PR. |
|
Claude finished @justin808's task in 1m 44s —— View job Code Review CompleteI've reviewed PR #893 which adds Overall AssessmentThe implementation is clean, focused, and functional. The logic correctly handles the new Critical Issue: Missing CHANGELOG EntryBLOCKING - According to CLAUDE.md, this PR requires a CHANGELOG.md entry:
This is a user-visible feature that should be documented. Based on the existing format in CHANGELOG.md, it should be added to the Suggested entry: ### Added
- **Added SKIP=true installer option to preserve existing files**. [PR #893](https://github.com/shakacode/shakapacker/pull/893) by [justin808](https://github.com/justin808). When running `bundle exec rake shakapacker:install`, you can now use `SKIP=true` to preserve existing files and only create missing ones, complementing the existing `FORCE=true` option. If both are set, `FORCE` takes precedence.Code Quality Observations1. Variable Naming (Minor - Not Blocking)The variable name lib/install/template.rb:10-16 and lib/install/binstubs.rb:1-7 # Current (slightly misleading):
force_option = if ENV["FORCE"]
{ force: true }
elsif ENV["SKIP"]
{ skip: true }
else
{}
end
# Suggested (clearer):
conflict_option = if ENV["FORCE"]
{ force: true }
elsif ENV["SKIP"]
{ skip: true }
else
{}
endThis would require updating 3 usages in template.rb (lines 40, 62) and 1 usage in binstubs.rb (line 10). 2. Documentation Enhancement (Optional)The README.md documentation is clear, but could mention the precedence behavior as noted in the PR description: README.md:169 # Current:
You can either approve all the prompts for overriding these files, use `FORCE=true` to overwrite without prompting, or use `SKIP=true` to preserve existing files and only create missing ones.
# Suggested addition:
You can either approve all the prompts for overriding these files, use `FORCE=true` to overwrite without prompting, or use `SKIP=true` to preserve existing files and only create missing ones. If both are set, `FORCE=true` takes precedence.Positive Observations✅ Correct precedence logic - Testing RecommendationsSince the PR notes that bundle-based checks couldn't be run locally due to Ruby version constraints, consider testing:
SummaryRecommendation: Request CHANGELOG.md update before merging. The code implementation itself is solid and ready. Required:
Optional improvements:
|
Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com>
Renames the variable to better reflect its purpose now that it handles both FORCE and SKIP modes. Adds a note that FORCE takes precedence when both are set. Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com>
There was a problem hiding this comment.
Actionable comments posted: 1
🤖 Fix all issues with AI agents
In `@lib/install/template.rb`:
- Around line 10-16: The code currently treats any set ENV["SKIP"] or
ENV["FORCE"] as truthy (so "false" or "0" still activates), so update the
conflict_option logic to test for explicit truthy strings (e.g. check
ENV["SKIP"] && %w[true 1 yes].include?(ENV["SKIP"].to_s.downcase)) and do the
same for ENV["FORCE"] (or extract a small helper like truthy_env?(name)) so only
intended values like "true"/"1"/"yes" enable skip/force in the conflict_option
branch.
🧹 Nitpick comments (1)
lib/install/binstubs.rb (1)
1-7: Duplicatedconflict_optionlogic withtemplate.rb.This block is identical to lines 10–16 in
template.rb. Consider extracting it into a shared helper (e.g., inlib/install/shared.rbor a small utility module) to keep the two files in sync. Not urgent since the block is small, but it would prevent future divergence.
| conflict_option = if ENV["FORCE"] | ||
| { force: true } | ||
| elsif ENV["SKIP"] | ||
| { skip: true } | ||
| else | ||
| {} | ||
| end |
There was a problem hiding this comment.
ENV["SKIP"] is truthy for any value, including "false" or "0".
ENV["SKIP"] returns a non-nil string for any set value, so SKIP=false bundle exec rake shakapacker:install would still activate skip mode. Consider checking for explicit truthy values to match the documented SKIP=true usage. The same applies to ENV["FORCE"], though that's pre-existing behavior.
Proposed fix
-conflict_option = if ENV["FORCE"]
+conflict_option = if ENV["FORCE"] == "true"
{ force: true }
-elsif ENV["SKIP"]
+elsif ENV["SKIP"] == "true"
{ skip: true }
else
{}
end🤖 Prompt for AI Agents
In `@lib/install/template.rb` around lines 10 - 16, The code currently treats any
set ENV["SKIP"] or ENV["FORCE"] as truthy (so "false" or "0" still activates),
so update the conflict_option logic to test for explicit truthy strings (e.g.
check ENV["SKIP"] && %w[true 1 yes].include?(ENV["SKIP"].to_s.downcase)) and do
the same for ENV["FORCE"] (or extract a small helper like truthy_env?(name)) so
only intended values like "true"/"1"/"yes" enable skip/force in the
conflict_option branch.
1 participant
Summary
SKIP=truesupport to installer file operations inlib/install/template.rbSKIP=truebehavior for binstub copying inlib/install/binstubs.rbFORCE=truevsSKIP=trueBehavior
FORCE=true: overwrite conflicting filesSKIP=true: keep existing files and only create missing filesFORCEwinsCloses #889.
Validation
Summary by CodeRabbit
New Features
Documentation