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

Already on GitHub? Sign in to your account

Push add column options to schema creation #10425

Merged
merged 1 commit into from Jun 13, 2013

Conversation

Projects
None yet
4 participants
Contributor

ranjaykrishna commented May 2, 2013

refactoring code in activerecord

@rafaelfranca rafaelfranca commented on the diff May 2, 2013

..._record/connection_adapters/abstract_mysql_adapter.rb
@@ -659,10 +668,9 @@ def translate_exception(exception, message)
end
def add_column_sql(table_name, column_name, type, options = {})
- add_column_sql = "ADD #{quote_column_name(column_name)} #{type_to_sql(type, options[:limit], options[:precision], options[:scale])}"
- add_column_options!(add_column_sql, options)
- add_column_position!(add_column_sql, options)
- add_column_sql
+ td = create_table_definition table_name, options[:temporary], options[:options]
+ cd = td.new_column_definition(column_name, type, options)
+ schema_creation.visit_AddColumn cd
@rafaelfranca

rafaelfranca May 2, 2013

Owner

Should not we build a AddColumn object and call schema_creation.accept? I don't think we should know which visitor method we have to call

@jeran

jeran May 2, 2013

Contributor

Last I spoke with @tenderlove on this subject, he suggested to make visit_AddColumn public for now. Aaron, what do you think about adding another object to schema_definitions to handle this?

@tenderlove

tenderlove May 22, 2013

Owner

@rafaelfranca I think we should add the object eventually (we just don't have it right now).

@rafaelfranca

rafaelfranca May 22, 2013

Owner

Seems fine.

Contributor

ranjaykrishna commented Jun 12, 2013

Squashed all the commits into one. @tenderlove , please merge the branch whenever you have the time. If there is anything else that needs to be done, please let us know.

@rafaelfranca rafaelfranca commented on an outdated diff Jun 12, 2013

..._record/connection_adapters/abstract_mysql_adapter.rb
end
def rename_column_sql(table_name, column_name, new_column_name)
- options = {}
+ options = {:name => new_column_name}
@rafaelfranca

rafaelfranca Jun 12, 2013

Owner

Just a minor comment, use the new hash syntax:

options = { name: new_column_name }
@jeran jeran Moving add_column_options! up to SchemaCreation
removed two instances of add_column_options! from abstract_mysql_adapter

reworked rename_column_sql to remove add_column_options from schema_statements

changed to use new hash syntax.
ea72430
Contributor

ranjaykrishna commented Jun 13, 2013

Just changed the syntax to use the new hash function. anything else left before merging?

@rafaelfranca rafaelfranca added a commit that referenced this pull request Jun 13, 2013

@rafaelfranca rafaelfranca Merge pull request #10425 from ranjaykrishna/push_add_column_options_…
…to_schema_creation

Push add column options to schema creation
47e8bb1

@rafaelfranca rafaelfranca merged commit 47e8bb1 into rails:master Jun 13, 2013

@yahonda yahonda added a commit to yahonda/oracle-enhanced that referenced this pull request Jul 1, 2013

@yahonda yahonda Support add_column_options 7d2a0eb

@yahonda yahonda referenced this pull request in rsim/oracle-enhanced Dec 3, 2013

Merged

Address test create table with defaults #384

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment