Refactor db:structure:dump task. #6782

Merged
merged 1 commit into from Jun 19, 2012

Projects

None yet

2 participants

@kennyj
kennyj commented Jun 19, 2012

I'm also inspired by #6761. I've moved the logic for db:structure:dump into the task classes.

FYI: I'm trying to solve #5547, and #6648 was my first PR for this problem.

@rafaelfranca
Member

@kennyj seems fine, also remember to link the issue that you was trying to solve.

@rafaelfranca rafaelfranca commented on the diff Jun 19, 2012
activerecord/lib/active_record/tasks/database_tasks.rb
@@ -60,6 +60,12 @@ def purge(configuration)
class_for_adapter(configuration['adapter']).new(configuration).purge
end
+ def structure_dump(*arguments)
@rafaelfranca
rafaelfranca Jun 19, 2012 Ruby on Rails member

as we only accepts two arguments I think that is better to use they explicitly here.

@kennyj
kennyj Jun 19, 2012

My first version was more explicitly.
But in only sqlite testcase, we need to pass Rails.root ...

What do you think about the following signature ?

def structure_dump(configuration, filename, root = Rails.root)
@rafaelfranca
rafaelfranca Jun 19, 2012 Ruby on Rails member

I don't think that this will work. We really need to pass Rails.root to the initializer?

@kennyj
kennyj Jun 19, 2012

If we don't pass root_path, we'll face the following an exception.

test_structure_dump(ActiveRecord::SqliteStructureDumpTest):
NameError: uninitialized constant ActiveRecord::Tasks::SQLiteDatabaseTasks::Rails
    /home/kennyj/rails/activerecord/lib/active_record/tasks/sqlite_database_tasks.rb:7:in `initialize'
...

and related code

  1 module ActiveRecord
  2   module Tasks # :nodoc:
  3     class SQLiteDatabaseTasks # :nodoc:
  4
  5       delegate :connection, :establish_connection, to: ActiveRecord::Base
  6
  7       def initialize(configuration, root = Rails.root)
  8         @configuration, @root = configuration, root
  9       end
 10

So when executing testcase in sqlite, I think we must pass a path to structure_dump method.
Please see also https://github.com/rails/rails/blob/master/activerecord/test/cases/sqlite_rake_test.rb (we always pass a path).

@kennyj
kennyj Jun 19, 2012

Certainly, my snippet isn't work ;-)

@rafaelfranca
rafaelfranca Jun 19, 2012 Ruby on Rails member

I think that should be

def initialize(configuration, root = ::Rails.root)
  @configuration, @root = configuration, root
end
@kennyj
kennyj Jun 19, 2012

NameError: uninitialized constant Rails ...

I guess we need this default argument to separate Rails from AR.

@rafaelfranca
rafaelfranca Jun 19, 2012 Ruby on Rails member

Ok.

@rafaelfranca
Member

Oops, this one is not trying to change the mysql dump task, is only a refactoring.

@kennyj
kennyj commented Jun 19, 2012

@rafaelfranca
What do you means ? Please tell me more detalis. I think I didn't change the behavior.

@rafaelfranca
Member

@kennyj yes, I thought that you added the code for the mysqldump pull request.

@kennyj
kennyj commented Jun 19, 2012

@rafaelfranca You are assured of that problem. It is my next challenge :-)

@rafaelfranca
Member

@kennyj all tests are passing?

@kennyj
kennyj commented Jun 19, 2012

rake test_sqlite3 test_mysql2 test_postgresql is all green.
And cd railties; ruby -Ilib:test test/application/rake_test.rb is also all green.

So I guess all tests are passing.

@rafaelfranca rafaelfranca merged commit 9edeb32 into rails:master Jun 19, 2012
@kennyj
kennyj commented Jun 19, 2012

thanks @rafaelfranca .
I'll try to structure:load tomorrow. good night :)

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