Bugfix: Prevent generators from overwriting files #274
Conversation
maxemitchell
force-pushed
the
prevent_file_overwriting
branch
from
6c2d4ae
to
3f32755
Compare
| @@ -88,7 +151,7 @@ class #{inflector.camelize(action)} < #{inflector.camelize(app)}::View | |||
| EXPECTED | |||
|
|
|||
| expect(fs.read("app/templates/#{controller}/#{action}.html.erb")).to eq(template_file) | |||
| expect(output).to include("Created app/views/#{controller}/#{action}.rb") | |||
| expect(output).to include("Created app/templates/#{controller}/#{action}.html.erb") | |||
There was a problem hiding this comment.
this was looking for the wrong file
There was a problem hiding this comment.
Nice find indeed! How did this even pass before? 😅
| @@ -305,47 +375,45 @@ class #{inflector.camelize(action)} < #{inflector.camelize(app)}::View | |||
| EXPECTED | |||
|
|
|||
| expect(fs.read("app/templates/#{controller}/#{action}.html.erb")).to eq(template_file) | |||
| expect(output).to include("Created app/views/#{controller}/#{action}.rb") | |||
| expect(output).to include("Created app/templates/#{controller}/#{action}.html.erb") | |||
There was a problem hiding this comment.
same thing here with wrong file
| context "with nested action name" do | ||
| let(:context) { Hanami::CLI::Generators::App::ActionContext.new(inflector, app, nil, %w[api users], "thing") } |
| end | ||
| end | ||
|
|
||
| context "deeply nested action" do |
There was a problem hiding this comment.
this deeply nested action context was duplicate
There was a problem hiding this comment.
Thank you for taking care of this, @maxemitchell! Especially continuing the work on it even after RubyConf finished — truly heroic! 🤩
The changes here all look good to me 👍🏼
I'd be happy to merge this as is, because this PR alone is already a valuable improvement on what we're doing now.
Before I do that, I'd like to take this moment to get your thoughts on how this PR fits into the bigger picture.
With this PR in its current state, if a user ran a generator that encountered an existing file, the hanami CLI would crash with the new FileAlreadyExistsError and print out a stack trace. On the plus side, this prevents unwanted file overwriting, but the rest of the user experience here doesn't feel so good. The error and stacktrace will be overwhelming, and it may be hard for the user to identify the cause for the CLI command crashing.
I think a better experience here might be fore our generate command classes to catch this FileAlreadyExistsError and print a nicer error message before then cleanly exiting (with a non-zero exit code). This might be something we can put into a single base command class for all the generators, so that we don't have to implement it in every single command class.
What do you think about this? Any thoughts on how else this might be made nicer? And now the big question: is this something you'd also be interested in working on? 😇 I'd be very happy for it to be in a follow-up PR so that we can merge this one and stay focused on smaller, incremental changes. Let me know!
| @@ -88,7 +151,7 @@ class #{inflector.camelize(action)} < #{inflector.camelize(app)}::View | |||
| EXPECTED | |||
|
|
|||
| expect(fs.read("app/templates/#{controller}/#{action}.html.erb")).to eq(template_file) | |||
| expect(output).to include("Created app/views/#{controller}/#{action}.rb") | |||
| expect(output).to include("Created app/templates/#{controller}/#{action}.html.erb") | |||
There was a problem hiding this comment.
Nice find indeed! How did this even pass before? 😅
This adds a
createmethod that raises an error if the file already exists.This allows the
writemethod to still be used by specs and when we want to override (e.g. for editing the routes file).Addresses #181