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

Refactored remove_column #3707

Merged
merged 1 commit into from
May 2, 2012
Merged

Conversation

ebeigarts
Copy link
Contributor

ActiveRecord::Table#remove should use *column_names instead of column_names.

So, I have removed unnecessary flatten for column_names that also caused issues here.

@jonleighton
Copy link
Member

Unsure about this. I agree with the principle but there might be apps in the wild that are passing an array to remove_column. cc @tenderlove

@ebeigarts
Copy link
Contributor Author

At least it isn't documented anywhere that we can pass nested Arrays in column_names here.

@drogus
Copy link
Member

drogus commented Dec 10, 2011

I would add deprecation warning and merge it after next release

@carlosantoniodasilva
Copy link
Member

This should be ok for master now?

@carlosantoniodasilva
Copy link
Member

@ebeigarts hey, could you bring this up-to-date with master, so we can review it and continue the discussion? Thanks.

@jonleighton can we go ahead to merge this in master?

@jonleighton
Copy link
Member

We can merge this if a deprecation is added.

@drogus
Copy link
Member

drogus commented May 1, 2012

I've rebased that commit and added deprecation warning here: drogus@d7e60b6, @jonleighton does it look good, should I push those?

@jonleighton
Copy link
Member

@drogus seems fine

drogus added a commit that referenced this pull request May 2, 2012
@drogus drogus merged commit ae8f497 into rails:master May 2, 2012
@drogus
Copy link
Member

drogus commented May 2, 2012

I've deprecated this behavior here: 02ca915

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.

4 participants