fixing issue where destroy fails to delete file

danielmklein commented 2 years ago

Fixes #205

What is the purpose of this pull request?

As described in #205, currently when someone does rails generate logidze:model MyModel and then wants to revoke that generation with rails destroy logidze:model MyModel, the injected has_logidze is removed from the model file, but the created migration file is not deleted. This PR fixes that.

What changes did you make? (overview)

Remove the migration_file_name method from ModelGenerator entirely.

After digging, I discovered that this method was clobbering an attr_accessor of the same name in the underlying generator plumbing in Rails::Generators::Migration (lib/rails/generators/migration.rb in railties), which was causing the migration_exists? in that same file not to work as expected. This was, in turn, causing code in Rails::Generators::Actions::CreateMigration (lib/rails/generators/actions/create_migration.rb in railties) not to realize that there was already a matching migration file existing with a prior migration number (which we should delete), so it would fall back to using the "current" migration number, for which a file doesn't exist.

Is there anything you'd like reviewers to focus on?

Are there edge cases or things to consider that I'm not thinking about? I am very new to this codebase and thus fairly unfamiliar, so I have somewhat of "blinders" on and am focused on my specific use case.

Checklist

[X] I've added tests for this change
[X] I've added a Changelog entry
[X] I've updated a documentation (Readme)

palkan commented 2 years ago

Thanks!

miharekar commented 1 year ago

Shouldn't deleting create a new migration that drops the logidze trigger? 🤔

danielmklein commented 1 year ago

Shouldn't deleting create a new migration that drops the logidze trigger? 🤔

My two cents: that would not align with how eg the original rails destroy model MyModel works. Running rails destroy model MyModel deletes the model class file and the table create migration, but it doesn't generate a "drop table" migration, correct?

In other words, as I see it, rails destroy ... destroys the code files by the corresponding rails generate ... command, not the actual database etc objects.

miharekar commented 1 year ago

Makes sense. However I do think it might make sense to have a task that would revert the migration. Essentially running the down part.

palkan / logidze