Sexy dumper now has its groove on (closes #8281) [Chris Wanstrath]

git-svn-id: http://svn-commit.rubyonrails.org/rails/trunk@6719 5ecf4fe2-1ee6-0310-87b1-e25e094e27de

3 files changed, 44 insertions, 15 deletions

diff --git a/activerecord/CHANGELOG b/activerecord/CHANGELOG
index d0875adce0..7ca462abda 100644
--- a/activerecord/CHANGELOG
+++ b/activerecord/CHANGELOG
@@ -30,8 +30,6 @@
       t.string  :name, :value, :default => "Untitled"
       t.timestamps
     end
-    
-  Note: Schema dumping still happens in the old style -- someone care to update it?
 
 * Use association name for the wrapper element when using .to_xml.  Previous behavior lead to non-deterministic situations with STI and polymorphic associations. [Koz, jstrachan]
 
diff --git a/activerecord/lib/active_record/schema_dumper.rb b/activerecord/lib/active_record/schema_dumper.rb
index 98c1163458..7fe5f26daf 100644
--- a/activerecord/lib/active_record/schema_dumper.rb
+++ b/activerecord/lib/active_record/schema_dumper.rb
@@ -89,22 +89,37 @@ HEADER
             raise StandardError, "Unknown type '#{column.sql_type}' for column '#{column.name}'" if @types[column.type].nil?
             next if column.name == pk
             spec = {}
-            spec[:name]    = column.name.inspect
-            spec[:type]    = column.type.inspect
-            spec[:limit]   = column.limit.inspect if column.limit != @types[column.type][:limit] && column.type != :decimal
+            spec[:name]      = column.name.inspect
+            spec[:type]      = column.type.to_s
+            spec[:limit]     = column.limit.inspect if column.limit != @types[column.type][:limit] && column.type != :decimal
             spec[:precision] = column.precision.inspect if !column.precision.nil?
-            spec[:scale] = column.scale.inspect if !column.scale.nil?
-            spec[:null]    = 'false' if !column.null
-            spec[:default] = default_string(column.default) if !column.default.nil?
+            spec[:scale]     = column.scale.inspect if !column.scale.nil?
+            spec[:null]      = 'false' if !column.null
+            spec[:default]   = default_string(column.default) if !column.default.nil?
             (spec.keys - [:name, :type]).each{ |k| spec[k].insert(0, "#{k.inspect} => ")}
             spec
           end.compact
-          keys = [:name, :type, :limit, :precision, :scale, :default, :null] & column_specs.map{ |spec| spec.keys }.inject([]){ |a,b| a | b }
+
+          # find all migration keys used in this table
+          keys = [:name, :limit, :precision, :scale, :default, :null] & column_specs.map(&:keys).flatten
+
+          # figure out the lengths for each column based on above keys
           lengths = keys.map{ |key| column_specs.map{ |spec| spec[key] ? spec[key].length + 2 : 0 }.max }
-          format_string = lengths.map{ |len| "%-#{len}s" }.join("")
+
+          # the string we're going to sprintf our values against, with standardized column widths
+          format_string = lengths.map{ |len| "%-#{len}s" }
+
+          # find the max length for the 'type' column, which is special
+          type_length = column_specs.map{ |column| column[:type].length }.max
+
+          # add column type definition to our format string
+          format_string.unshift "    t.%-#{type_length}s "
+
+          format_string *= ''
+
           column_specs.each do |colspec|
             values = keys.zip(lengths).map{ |key, len| colspec.key?(key) ? colspec[key] + ", " : " " * len }
-            tbl.print "    t.column "
+            values.unshift colspec[:type]
             tbl.print((format_string % values).gsub(/,\s*$/, ''))
             tbl.puts
           end
diff --git a/activerecord/test/schema_dumper_test.rb b/activerecord/test/schema_dumper_test.rb
index 576c2cebc3..d18b99d561 100644
--- a/activerecord/test/schema_dumper_test.rb
+++ b/activerecord/test/schema_dumper_test.rb
@@ -32,11 +32,27 @@ if ActiveRecord::Base.connection.respond_to?(:tables)
       return assert(true) if matches.empty?
       assert_equal 1, matches.map{ |match| match.offset(0).first }.uniq.length
     end
+
+    def column_definition_lines(output = standard_dump)
+      output.scan(/^( *)create_table.*?\n(.*?)^\1end/m).map{ |m| m.last.split(/\n/) }
+    end
+
+    def test_types_line_up
+      column_definition_lines.each do |column_set|
+        next if column_set.empty?
+
+        lengths = column_set.map do |column| 
+          if match = column.match(/t\.(?:integer|decimal|float|datetime|timestamp|time|date|text|binary|string|boolean)\s+"/)
+            match[0].length
+          end
+        end
+
+        assert_equal 1, lengths.uniq.length
+      end
+    end
     
     def test_arguments_line_up
-      output  = standard_dump
-      output.scan(/^( *)create_table.*?\n(.*?)^\1end/m).map{ |m| m.last.split(/\n/) }.each do |column_set|
-        assert_line_up(column_set, /:(?:integer|decimal|float|datetime|timestamp|time|date|text|binary|string|boolean)/, true)
+      column_definition_lines.each do |column_set|
         assert_line_up(column_set, /:default => /)
         assert_line_up(column_set, /:limit => /)
         assert_line_up(column_set, /:null => /)
@@ -92,7 +108,7 @@ if ActiveRecord::Base.connection.respond_to?(:tables)
     if current_adapter?(:MysqlAdapter)
       def test_schema_dump_should_not_add_default_value_for_mysql_text_field
         output = standard_dump
-        assert_match %r{t.column "body",\s+:text,\s+:null => false$}, output
+        assert_match %r{t.text\s+"body",\s+:null => false$}, output
       end
     end