keep index names when using `alter_table` with sqlite3 #8522

Merged
merged 1 commit into from Dec 19, 2012

Conversation

Projects
None yet
3 participants
Member

senny commented Dec 15, 2012

The code in SQLite3Adapter#copy_table_indexes wrongly renamed indexes when using alter_table. This only happened for indexes, which begin with the table name. As soon as they had a prefix it worked.

This patch makes #copy_table_indexes aware that no prefix is required. It is a fix for #3489

Member

senny commented Dec 15, 2012

@rafaelfranca @steveklabnik can you take a look?

@@ -537,7 +537,6 @@ def copy_table(from, to, options = {}) #:nodoc:
end
yield @definition if block_given?
end
-
@steveklabnik

steveklabnik Dec 15, 2012

Member

unrelated whitespace change, not sure how much it matters.

@senny

senny Dec 15, 2012

Member

I normally clean trailing whitespace. I used to do separate commits but I think for such small portions it should be ok to have them in the diff. Do you think it's a problem?

@steveklabnik

steveklabnik Dec 15, 2012

Member

I'm never quite sure if this kind of thing counts as 'cosmetic' or not. Generally we just try to fix whitespace in the part of the code you're actually changing. I'm not sure what the Official Policy is about this weird middle case.

@carlosantoniodasilva

carlosantoniodasilva Dec 16, 2012

Owner

I believe that if you're touching a file and some whitespace is cleaned up (mostly because your editor is configured to remove extra spaces on save), it should be fine, I'd not bother.

Member

steveklabnik commented Dec 15, 2012

This looks totally reasonable to me but I'm not comfortable enough with this part of the code to merge.

Member

senny commented Dec 18, 2012

rebased. @carlosantoniodasilva @rafaelfranca what do you think?

activerecord/test/cases/migration/rename_column_test.rb
+ add_index :test_models, :category, :name => 'test_models_categories_idx'
+
+ assert_equal ['test_models_categories_idx'], connection.indexes('test_models').map(&:name)
+ change_column "test_models", "category", :string, :null => false, :default => 'article'
@carlosantoniodasilva

carlosantoniodasilva Dec 19, 2012

Owner

@senny can you change to 1.9 style hash, pls?

Looks good, just the 1.9 hash thing and I'll merge :)

Looks good, just the 1.9 hash thing and I'll merge :)

Member

senny commented Dec 19, 2012

@carlosantoniodasilva updated to 1.9 hash syntax.

carlosantoniodasilva added a commit that referenced this pull request Dec 19, 2012

Merge pull request #8522 from senny/3489_index_names_on_copy
Leep index names when using `alter_table` with sqlite3. Closes #3489

@carlosantoniodasilva carlosantoniodasilva merged commit e36f9ef into rails:master Dec 19, 2012

There we go, thanks!

senny added a commit to senny/rails that referenced this pull request Dec 19, 2012

Backport #8522, Keep index names when using with sqlite3
Conflicts:

	activerecord/CHANGELOG.md
	activerecord/lib/active_record/connection_adapters/sqlite3_adapter.rb
	activerecord/test/cases/migration/rename_column_test.rb

carlosantoniodasilva added a commit that referenced this pull request Dec 19, 2012

Merge pull request #8558 from senny/backport_3489
Backport #8522, Keep index names when using  with sqlite3
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment