Database comments: switch to keyword args for new table options

* Switch to keyword args where we can without breaking compat.
* Use add_table_options! for :options, too.
* Some code polish.

activerecord/lib/active_record/connection_adapters/abstract/schema_creation.rb

@@ -53,9 +53,8 @@ def visit_TableDefinition(o)
               statements.concat(o.foreign_keys.map { |to_table, options| foreign_key_in_create(o.name, to_table, options) })
             end
 
-            create_sql << "(#{statements.join(', ')}) " if statements.present?
+            create_sql << "(#{statements.join(', ')})" if statements.present?
             add_table_options!(create_sql, table_options(o))
-            create_sql << "#{o.options}"
             create_sql << " AS #{@conn.to_sql(o.as)}" if o.as
             create_sql
           end
@@ -84,13 +83,16 @@ def visit_DropForeignKey(name)
           end
 
           def table_options(o)
-            options = {}
-            options[:comment] = o.comment
-            options
+            table_options = {}
+            table_options[:comment] = o.comment
+            table_options[:options] = o.options
+            table_options
           end
 
-          def add_table_options!(sql, _options)
-            sql
+          def add_table_options!(create_sql, options)
+            if options_sql = options[:options]
+              create_sql << " #{options_sql}"
+            end
           end
 
           def column_options(o)

activerecord/lib/active_record/connection_adapters/abstract/schema_definitions.rb

@@ -209,7 +209,7 @@ class TableDefinition
       attr_accessor :indexes
       attr_reader :name, :temporary, :options, :as, :foreign_keys, :comment
 
-      def initialize(name, temporary, options, as = nil, comment = nil)
+      def initialize(name, temporary = false, options = nil, as = nil, comment: nil)
         @columns_hash = {}
         @indexes = {}
         @foreign_keys = []

activerecord/lib/active_record/connection_adapters/abstract/schema_statements.rb

@@ -18,7 +18,7 @@ def table_options(table_name)
         nil
       end
 
-      # Returns comment associated with given table in database
+      # Returns the table comment that's stored in database metadata.
       def table_comment(table_name)
         nil
       end
@@ -259,8 +259,8 @@ def primary_key(table_name)
       #     SELECT * FROM orders INNER JOIN line_items ON order_id=orders.id
       #
       # See also TableDefinition#column for details on how to create columns.
-      def create_table(table_name, options = {})
-        td = create_table_definition table_name, options[:temporary], options[:options], options[:as], options[:comment]
+      def create_table(table_name, comment: nil, **options)
+        td = create_table_definition table_name, options[:temporary], options[:options], options[:as], comment: comment
 
         if options[:id] != false && !options[:as]
           pk = options.fetch(:primary_key) do
@@ -270,7 +270,7 @@ def create_table(table_name, options = {})
           if pk.is_a?(Array)
             td.primary_keys pk
           else
-            td.primary_key pk, options.fetch(:id, :primary_key), options.except(:comment)
+            td.primary_key pk, options.fetch(:id, :primary_key), options
           end
         end
 
@@ -289,7 +289,8 @@ def create_table(table_name, options = {})
         end
 
         if supports_comments? && !supports_comments_in_create?
-          change_table_comment(table_name, options[:comment]) if options[:comment]
+          change_table_comment(table_name, comment) if comment
+
           td.columns.each do |column|
             change_column_comment(table_name, column.name, column.comment) if column.comment
           end
@@ -1096,10 +1097,10 @@ def update_table_definition(table_name, base) #:nodoc:
         Table.new(table_name, base)
       end
 
-      def add_index_options(table_name, column_name, options = {}) #:nodoc:
+      def add_index_options(table_name, column_name, comment: nil, **options) #:nodoc:
         column_names = Array(column_name)
 
-        options.assert_valid_keys(:unique, :order, :name, :where, :length, :internal, :using, :algorithm, :type, :comment)
+        options.assert_valid_keys(:unique, :order, :name, :where, :length, :internal, :using, :algorithm, :type)
 
         index_type = options[:type].to_s if options.key?(:type)
         index_type ||= options[:unique] ? "UNIQUE" : ""
@@ -1127,8 +1128,6 @@ def add_index_options(table_name, column_name, options = {}) #:nodoc:
         end
         index_columns = quoted_columns_for_index(column_names, options).join(", ")
 
-        comment = options[:comment] if options.key?(:comment)
-
         [index_name, index_type, index_columns, index_options, algorithm, using, comment]
       end
 
@@ -1136,14 +1135,14 @@ def options_include_default?(options)
         options.include?(:default) && !(options[:null] == false && options[:default].nil?)
       end
 
-      # Adds comment for given table or drops it if +nil+ given
+      # Changes the comment for a table or removes it if +nil+.
       def change_table_comment(table_name, comment)
-        raise NotImplementedError, "change_table_comment is not implemented"
+        raise NotImplementedError, "#{self.class} does not support changing table comments"
       end
 
-      # Adds comment for given table column or drops it if +nil+ given
+      # Changes the comment for a column or removes it if +nil+.
       def change_column_comment(table_name, column_name, comment) #:nodoc:
-        raise NotImplementedError, "change_column_comment is not implemented"
+        raise NotImplementedError, "#{self.class} does not support changing column comments"
       end
 
       protected
@@ -1227,8 +1226,8 @@ def rename_column_indexes(table_name, column_name, new_column_name)
         end
 
       private
-      def create_table_definition(name, temporary = false, options = nil, as = nil, comment = nil)
-        TableDefinition.new(name, temporary, options, as, comment)
+      def create_table_definition(*args)
+        TableDefinition.new(*args)
       end
 
       def create_alter_table(name)

activerecord/lib/active_record/connection_adapters/abstract_adapter.rb

@@ -278,7 +278,7 @@ def supports_json?
         false
       end
 
-      # Does adapter supports comments on database objects (tables, columns, indexes)?
+      # Does this adapter support metadata comments on database objects (tables, columns, indexes)?
       def supports_comments?
         false
       end

activerecord/lib/active_record/connection_adapters/abstract_mysql_adapter.rb

@@ -160,8 +160,8 @@ def each_hash(result) # :nodoc:
         raise NotImplementedError
       end
 
-      def new_column(field, default, sql_type_metadata, null, table_name, default_function = nil, collation = nil, comment = nil) # :nodoc:
-        MySQL::Column.new(field, default, sql_type_metadata, null, table_name, default_function, collation, comment)
+      def new_column(*args) #:nodoc:
+        MySQL::Column.new(*args)
       end
 
       # Must return the MySQL error number from the exception, if the exception has an
@@ -394,20 +394,20 @@ def columns(table_name) # :nodoc:
           else
             default, default_function = field[:Default], nil
           end
-          new_column(field[:Field], default, type_metadata, field[:Null] == "YES", table_name, default_function, field[:Collation], field[:Comment].presence)
+          new_column(field[:Field], default, type_metadata, field[:Null] == "YES", table_name, default_function, field[:Collation], comment: field[:Comment].presence)
         end
       end
 
-      def table_comment(table_name)
+      def table_comment(table_name) # :nodoc:
         select_value(<<-SQL.strip_heredoc, 'SCHEMA')
           SELECT table_comment
-          FROM INFORMATION_SCHEMA.TABLES
-          WHERE table_name=#{quote(table_name)};
+          FROM information_schema.tables
+          WHERE table_name=#{quote(table_name)}
         SQL
       end
 
-      def create_table(table_name, options = {}) #:nodoc:
-        super(table_name, options.reverse_merge(:options => "ENGINE=InnoDB"))
+      def create_table(table_name, **options) #:nodoc:
+        super(table_name, options: 'ENGINE=InnoDB', **options)
       end
 
       def bulk_change_table(table_name, operations) #:nodoc:
@@ -881,8 +881,8 @@ def create_table_info(table_name) # :nodoc:
         create_table_info_cache[table_name] ||= select_one("SHOW CREATE TABLE #{quote_table_name(table_name)}")["Create Table"]
       end
 
-      def create_table_definition(name, temporary = false, options = nil, as = nil, comment = nil) # :nodoc:
-        MySQL::TableDefinition.new(name, temporary, options, as, comment)
+      def create_table_definition(*args) # :nodoc:
+        MySQL::TableDefinition.new(*args)
       end
 
       def integer_to_sql(limit) # :nodoc:

activerecord/lib/active_record/connection_adapters/column.rb

@@ -15,7 +15,7 @@ class Column
       # +default+ is the type-casted default value, such as +new+ in <tt>sales_stage varchar(20) default 'new'</tt>.
       # +sql_type_metadata+ is various information about the type of the column
       # +null+ determines if this column allows +NULL+ values.
-      def initialize(name, default, sql_type_metadata = nil, null = true, table_name = nil, default_function = nil, collation = nil, comment = nil)
+      def initialize(name, default, sql_type_metadata = nil, null = true, table_name = nil, default_function = nil, collation = nil, comment: nil)
         @name = name.freeze
         @table_name = table_name
         @sql_type_metadata = sql_type_metadata

activerecord/lib/active_record/connection_adapters/mysql/schema_creation.rb

@@ -25,10 +25,11 @@ def visit_ChangeColumnDefinition(o)
           add_column_position!(change_column_sql, column_options(o.column))
         end
 
-        def add_table_options!(sql, options)
+        def add_table_options!(create_sql, options)
           super
-          if options[:comment]
-            sql << "COMMENT #{quote(options[:comment])} "
+
+          if comment = options[:comment]
+            create_sql << " COMMENT #{quote(comment)}"
           end
         end
 
@@ -39,17 +40,21 @@ def column_options(o)
         end
 
         def add_column_options!(sql, options)
-          if options[:charset]
-            sql << " CHARACTER SET #{options[:charset]}"
+          if charset = options[:charset]
+            sql << " CHARACTER SET #{charset}"
           end
-          if options[:collation]
-            sql << " COLLATE #{options[:collation]}"
+
+          if collation = options[:collation]
+            sql << " COLLATE #{collation}"
           end
-          new_sql = super
-          if options[:comment]
-            new_sql << " COMMENT #{quote(options[:comment])}"
+
+          super
+
+          if comment = options[:comment]
+            sql << " COMMENT #{quote(comment)}"
           end
-          new_sql
+
+          sql
         end
 
         def add_column_position!(sql, options)
@@ -58,6 +63,7 @@ def add_column_position!(sql, options)
           elsif options[:after]
             sql << " AFTER #{quote_column_name(options[:after])}"
           end
+
           sql
         end
 

activerecord/lib/active_record/connection_adapters/postgresql/schema_statements.rb

@@ -225,27 +225,27 @@ def columns(table_name) # :nodoc:
             type_metadata = fetch_type_metadata(column_name, type, oid, fmod)
             default_value = extract_value_from_default(default)
             default_function = extract_default_function(default_value, default)
-            new_column(column_name, default_value, type_metadata, !notnull, table_name, default_function, collation, comment)
+            new_column(column_name, default_value, type_metadata, !notnull, table_name, default_function, collation, comment: comment)
           end
         end
 
-        def new_column(name, default, sql_type_metadata, null, table_name, default_function = nil, collation = nil, comment = nil) # :nodoc:
-          PostgreSQLColumn.new(name, default, sql_type_metadata, null, table_name, default_function, collation, comment)
+        def new_column(*args) # :nodoc:
+          PostgreSQLColumn.new(*args)
         end
 
         # Returns a comment stored in database for given table
         def table_comment(table_name) # :nodoc:
           name = Utils.extract_schema_qualified_name(table_name.to_s)
-          return nil unless name.identifier
-
-          select_value(<<-SQL.strip_heredoc, 'SCHEMA')
-            SELECT pg_catalog.obj_description(c.oid, 'pg_class')
-            FROM pg_catalog.pg_class c
-              LEFT JOIN pg_namespace n ON n.oid = c.relnamespace
-            WHERE c.relname = '#{name.identifier}'
-              AND c.relkind IN ('r') -- (r)elation/table
-              AND n.nspname = #{name.schema ? "'#{name.schema}'" : 'ANY (current_schemas(false))'}
-          SQL
+          if name.identifier
+            select_value(<<-SQL.strip_heredoc, 'SCHEMA')
+              SELECT pg_catalog.obj_description(c.oid, 'pg_class')
+              FROM pg_catalog.pg_class c
+                LEFT JOIN pg_namespace n ON n.oid = c.relnamespace
+              WHERE c.relname = #{quote(name.identifier)}
+                AND c.relkind IN ('r') -- (r)elation/table
+                AND n.nspname = #{name.schema ? quote(name.schema) : 'ANY (current_schemas(false))'}
+            SQL
+          end
         end
 
         # Returns the current database name.
@@ -536,9 +536,9 @@ def rename_column(table_name, column_name, new_column_name) #:nodoc:
 
         def add_index(table_name, column_name, options = {}) #:nodoc:
           index_name, index_type, index_columns, index_options, index_algorithm, index_using, comment = add_index_options(table_name, column_name, options)
-          result = execute "CREATE #{index_type} INDEX #{index_algorithm} #{quote_column_name(index_name)} ON #{quote_table_name(table_name)} #{index_using} (#{index_columns})#{index_options}"
-          execute "COMMENT ON INDEX #{quote_column_name(index_name)} IS #{quote(comment)}" if comment
-          result # Result of execute is used in tests in activerecord/test/cases/adapters/postgresql/active_schema_test.rb
+          execute("CREATE #{index_type} INDEX #{index_algorithm} #{quote_column_name(index_name)} ON #{quote_table_name(table_name)} #{index_using} (#{index_columns})#{index_options}").tap do
+            execute "COMMENT ON INDEX #{quote_column_name(index_name)} IS #{quote(comment)}" if comment
+          end
         end
 
         def remove_index(table_name, options = {}) #:nodoc:

activerecord/lib/active_record/connection_adapters/postgresql_adapter.rb

@@ -163,6 +163,10 @@ def supports_comments?
         true
       end
 
+      def supports_comments_in_create?
+        false
+      end
+
       def index_algorithms
         { concurrently: 'CONCURRENTLY' }
       end
@@ -751,8 +755,8 @@ def extract_table_ref_from_insert_sql(sql) # :nodoc:
           $1.strip if $1
         end
 
-        def create_table_definition(name, temporary = false, options = nil, as = nil, comment = nil) # :nodoc:
-          PostgreSQL::TableDefinition.new(name, temporary, options, as, comment)
+        def create_table_definition(*args) # :nodoc:
+          PostgreSQL::TableDefinition.new(*args)
         end
 
         def can_perform_case_insensitive_comparison_for?(column)

activerecord/lib/active_record/schema_dumper.rb

@@ -138,8 +138,9 @@ def table(table, stream)
           table_options = @connection.table_options(table)
           tbl.print ", options: #{table_options.inspect}" unless table_options.blank?
 
-          comment = @connection.table_comment(table)
-          tbl.print ", comment: #{comment.inspect}" if comment
+          if comment = @connection.table_comment(table)
+            tbl.print ", comment: #{comment.inspect}"
+          end
 
           tbl.puts " do |t|"
 

activerecord/test/cases/comment_test.rb

@@ -26,8 +26,7 @@ def setup
     @connection.drop_table 'commenteds', if_exists: true
   end
 
-  if current_adapter?(:Mysql2Adapter, :PostgreSQLAdapter)
-
+  if ActiveRecord::Base.connection.supports_comments?
     def test_column_created_in_block
       Commented.reset_column_information
       column = Commented.columns_hash['name']
@@ -86,6 +85,5 @@ def test_schema_dump_with_comments
       assert_match %r[add_index\s+.+\s+comment: "\\\"Very important\\\" index that powers all the performance.\\nAnd it's fun!"], output
       assert_match %r[add_index\s+.+\s+name: "idx_obvious",.+\s+comment: "We need to see obvious comments"], output
     end
-
   end
 end