fix: ensure that all file manipulations are applied#595
Merged
Conversation
cd8d1f6 to
9ce1cde
Compare
There was a problem hiding this comment.
Pull Request Overview
This PR improves file manipulation methods by introducing bang versions (e.g. append_to_file!, prepend_to_file!, insert_into_file!) that verify changes were applied and raise an error if not.
- Replace non-bang file manipulation calls with bang versions that check file content changes
- Update multiple variant templates and configuration files to use the new bang methods
Reviewed Changes
Copilot reviewed 27 out of 27 changed files in this pull request and generated no comments.
Show a summary per file
| File | Description |
|---|---|
| template.rb | New bang methods for appending, prepending, and inserting files with change-checks |
| variants/frontend-base/template.rb | Changed calls to prepend_to_file! in favor of non-bang version for consistency with error raising |
| variants/accessibility/spec/system/home_feature_spec.rb | Updated insert_into_file! call to ensure file change verification |
| variants/devise/template.rb | Replaced non-bang calls with bang versions in header and model updates |
| variants/backend-base/config/environments/production.rb | Updated insert_into_file! calls for production environment configuration |
| variants/accessibility/Gemfile.rb | Updated insert_into_file! call in Gemfile to raise error on unchanged content |
| variants/backend-base/config.ru.rb | Replaced insert_into_file with bang version for Rack configuration |
| variants/deploy_with_ackama_ec2_capistrano/template.rb | Replaced append, insert, and prepend calls with bang versions |
| variants/bullet/template.rb | Replaced non-bang calls with bang versions for Gemfile and environment files |
| variants/backend-base/config/application.rb | Replaced insert_into_file calls with bang versions for application configuration |
| variants/deploy_with_capistrano/template.rb | Replaced file manipulation calls with bang versions to enforce content change |
| variants/backend-base/config/environments/development.rb | Updated insert_into_file! call for development environment configuration |
| variants/frontend-base/sentry/template.rb | Updated prepend and append calls with bang versions for Sentry integration |
| variants/code-annotation/template.rb | Updated insert_into_file! call for adding code annotation gems |
| variants/backend-base/Rakefile.rb | Updated append_to_file! call on the Rakefile |
| variants/accessibility/spec/rails_helper.rb | Updated several insert_into_file! calls for RSpec helper configuration |
| variants/frontend-base-typescript/template.rb | Replaced append_to_file call with bang version in CI script |
| variants/backend-base/config/environments/test.rb | Replaced insert_into_file call with bang version for test environment configuration |
| variants/audit-logging/template.rb | Replaced file manipulation calls with bang versions in audit logging templates |
| variants/frontend-base/js-lint/template.rb | Updated multiple append_to_file! calls in the CI script for linting |
Comments suppressed due to low confidence (1)
template.rb:496
- Consider adding tests to verify that append_to_file! (as well as its prepend and insert counterparts) correctly raises an error when the file content remains unchanged after the operation.
def append_to_file!(path, ...)
eefa17a to
3ee6d86
Compare
Contributor
Author
|
I've cherry-picked this into #573 and it errored, confirming my suspicion so I think this is worth landing |
G-Rath
added a commit
that referenced
this pull request
Mar 20, 2025
…erly match (#596) The current regexp will never work because it's expecting a double quote character after the end of the line, which is impossible for two reasons. Instead, I've replaced both characters with a newline as that feels like a better match (though really in hindsight just removing the double quote probably would have been enough to fix this 😅) Found via #595
3ee6d86 to
aefbcaf
Compare
JacobBriggsAckama
approved these changes
Mar 27, 2025
eoinkelly
reviewed
Mar 27, 2025
| end | ||
|
|
||
| def append_to_file!(path, ...) | ||
| content = File.binread(path) |
Contributor
There was a problem hiding this comment.
suggestion:non-blocking: I can't think of a situation where we would want to use this on a non utf-8 file. If a file can't be parsed as UTF-8 then it's probably an error so File.read is probably a bit better.
eoinkelly
approved these changes
Mar 27, 2025
G-Rath
added a commit
that referenced
this pull request
Apr 1, 2025
I realised right after I landed #595 that I'd forgotten to double check after rebasing that I've replaced new usages of the file manipulation methods, but then as I was preparing this I _also_ realized that we're meant to be prepending the `locals` comments into the templates 🤦
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
The file manipulation methods provided by Thor don't stop execution when they fail to do the defined manipulation, instead in our case they print an error but then proceed; we actually want to error in this situation since the whole point of our template is to be applying our preferred practices meaning its moot if we don't...
This really came into light with the Rails 8 upgrade which had a number of changes to the configuration files in particular that caused a number of our customizations to be skipped.
By introducing bang versions of the methods that error if the file contents does not change, we can ensure going forward all manipulations actually work; this uses the same logic I introduced in #533 for
gsub_file, and caught a bug that I fixed in #596