Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Add drop_enum command for Postgres #45735

Merged
merged 1 commit into from Aug 4, 2022
Merged

Conversation

ghiculescu
Copy link
Member

Follow up to #41469

This PR adds support for drop_enum, so you can drop enum types explicitly or through a db:migrate:down.

@yahonda
Copy link
Member

yahonda commented Aug 3, 2022

  1. What do you think about replacing DROP TYPE sql statements with drop_enum?
% git grep -n 'DROP TYPE'
activerecord/test/cases/adapters/postgresql/composite_test.rb:35:    @connection.execute "DROP TYPE IF EXISTS full_address"
activerecord/test/cases/adapters/postgresql/enum_test.rb:34:    @connection.execute "DROP TYPE IF EXISTS mood"
activerecord/test/cases/adapters/postgresql/postgresql_adapter_test.rb:405:        @connection.execute "DROP TYPE IF EXISTS feeling"
activerecord/test/cases/adapters/postgresql/range_test.rb:101:    @connection.execute "DROP TYPE IF EXISTS floatrange"
activerecord/test/cases/adapters/postgresql/range_test.rb:102:    @connection.execute "DROP TYPE IF EXISTS stringrange"
guides/source/active_record_postgresql.md:270:    DROP TYPE article_status;
%
  1. How about adding tests with explicit schema name? As we added explicit schema name option for create__enum via Support explicit schemas in PostgreSQL's create_enum #45707.

@ghiculescu
Copy link
Member Author

@yahonda several of those (composite_test and range_test) are not enums, but other examples of custom types. Do you think I should add a more general drop_type? It would do the same thing in Postgres but the name would be more correct.

I made the other changes as suggested - thanks for the schema reminder.

@yahonda
Copy link
Member

yahonda commented Aug 3, 2022

Thanks for the update.

Do you think I should add a more general drop_type? It would do the same thing in Postgres but the name would be more correct.

No, not necessary. I just git grep but did not verify each lines in detail.

yahonda
yahonda previously approved these changes Aug 3, 2022
@yahonda yahonda self-requested a review August 3, 2022 03:14
@@ -31,7 +31,7 @@ def setup

teardown do
@connection.drop_table "postgresql_enums", if_exists: true
@connection.execute "DROP TYPE IF EXISTS mood"
@connection.drop_enum "mood"
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

What do you think about adding if_exists: true support option for drop_enum?

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Oh yeah - good idea 👍

@yahonda yahonda dismissed their stale review August 3, 2022 03:21

Added another review comment.

@ghiculescu ghiculescu force-pushed the drop-enum branch 2 times, most recently from 7caffde to 85b3274 Compare August 3, 2022 03:33
yahonda
yahonda previously approved these changes Aug 3, 2022
@ghiculescu
Copy link
Member Author

Thanks @yahonda

@yahonda
Copy link
Member

yahonda commented Aug 3, 2022

Would you address the CHANGELOG.md conflicts?

@ghiculescu
Copy link
Member Author

Yep 👍

@yahonda yahonda merged commit a0c1d33 into rails:main Aug 4, 2022
@ghiculescu ghiculescu deleted the drop-enum branch August 4, 2022 15:16
jonathanhefner added a commit to jonathanhefner/rails that referenced this pull request Aug 5, 2022
Follow-up to rails#45735.

Prior to this commit, `drop_enum` could not be reversed when called with
the `if_exists: true` option, because `create_enum` could not accept
options.

This commit fixes that and adds test coverage.

This commit also ensures that reversing `drop_enum` will raise an
`ActiveRecord::IrreversibleMigration` error if no enum values are
specified.
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

2 participants