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 rename and rename_belongs_to for migrations. #366

Merged
merged 3 commits into from
May 13, 2020

Conversation

wout
Copy link
Contributor

@wout wout commented May 12, 2020

A proposal as suggested in #365. This makes me think about picking up #321 as well, since the same is true.

Copy link
Member

@paulcsmith paulcsmith left a comment

Choose a reason for hiding this comment

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

Love it 👍

Could you add that a migration that uses these two new methods? This will test that they work when actually used with Postgres since the spec suite runs all migrations.

@wout
Copy link
Contributor Author

wout commented May 13, 2020

Good you asked for the migration. As it turns out, you can only do one RENAME per ALTER TABLE statement. I didn't know that. 😊

https://stackoverflow.com/questions/23274679/renaming-multiple-columns-in-one-statement-with-postgresql

I chose to execute RENAME statements before ADD statements because that way you can do something like this:

alter table_for(User) do
  rename :name, :first_name
  add name : String
end

In other words, you can rename an existing column, and add a new one with the original name of the existing column, all in one migration. So, that's why I moved RENAME statements to the top.

Although not the same issue, I also included the conversion of the rename method to a macro here as well. It's a small fix in the same section of code. I hope you don't mind.

Copy link
Member

@paulcsmith paulcsmith left a comment

Choose a reason for hiding this comment

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

Looks great!

@@ -166,13 +168,36 @@ class Avram::Migrator::AlterTableStatement
]
end

def remove(name : Symbol)
dropped_rows << " DROP #{name.to_s}"
{% symbol_expected_message = "%s expected a symbol like ':user', instead got: '%s'" %}
Copy link
Member

Choose a reason for hiding this comment

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

This is a really cool idea 👍

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Yeah, I was really happy with that one. 😊

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants