Skip to content

HTTPS clone URL

Subversion checkout URL

You can clone with HTTPS or Subversion.

Download ZIP

Loading…

Refactor db:charset task #6779

Merged
merged 1 commit into from

3 participants

@simonjefford

In the spirit of #6761 I've moved the logic for db:charset into the task classes.

@pat

I like it, matches what I was thinking. Only thing I'd change - probably don't want to use each_current_configuration - that'll invoke it for both development and test if it's the former. Just use the passed in environment as the key against the hash of all configurations?

activerecord/lib/active_record/tasks/database_tasks.rb
@@ -47,6 +47,16 @@ def drop_current(environment = Rails.env)
}
end
+ def charset_current(environment = Rails.env)
+ configuration = ActiveRecord::Base.configurations[environment]
+ return charset(configuration)

No need for return here.

@pat
pat added a note

Indeed - make it a one-liner:

charset ActiveRecord::Base.configurations[environment]
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
@carlosantoniodasilva carlosantoniodasilva commented on the diff
activerecord/test/cases/mysql_rake_test.rb
((5 lines not shown))
+ def setup
+ @connection = stub(:create_database => true)
+ @configuration = {
+ 'adapter' => 'mysql',
+ 'database' => 'my-app-db'
+ }
+
+ ActiveRecord::Base.stubs(:connection).returns(@connection)
+ ActiveRecord::Base.stubs(:establish_connection).returns(true)
+ end
+
+ def test_db_retrieves_charset
+ @connection.expects(:charset)
+ ActiveRecord::Tasks::DatabaseTasks.charset @configuration
+ end
+ end

Just for consistency, could you move this and other db (sqlite/postgresql) charset tests to the end of the file instead? (like with DatabaseTasksCharsetTest). Also there's a typo in the class name: Charst => Charset.

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

Great! Can you check the comments and squash the commits, so we can merge? Thanks.

@simonjefford

I've made the changes suggested. What's the best way to push the squashed commits for you to merge - to a new branch?

activerecord/test/cases/postgresql_rake_test.rb
((6 lines not shown))
+ def setup
+ @connection = stub(:create_database => true)
+ @configuration = {
+ 'adapter' => 'postgresql',
+ 'database' => 'my-app-db'
+ }
+
+ ActiveRecord::Base.stubs(:connection).returns(@connection)
+ ActiveRecord::Base.stubs(:establish_connection).returns(true)
+ end
+
+ def test_db_retrieves_charset
+ @connection.expects(:encoding)
+ ActiveRecord::Tasks::DatabaseTasks.charset @configuration
+ end
+ end

I think that one is inside PostgreSQLPurgeTest test class, based on the indentation. Can you check?

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

@simonjefford you can push force to the same branch with the squashed commit and github will update the pull request accordingly.

@simonjefford simonjefford Refactor db:charset task
In a similar vein to Pat's work on create, drop etc, the db:charset
task is now a one liner in databases.rake
363ab88
@simonjefford

I did not know that, cool! I've moved the misplaced test and squashed.

@carlosantoniodasilva

Great, merging, thank you! :)

@carlosantoniodasilva carlosantoniodasilva merged commit 1aa052c into from
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Commits on Jun 19, 2012
  1. @simonjefford

    Refactor db:charset task

    simonjefford authored
    In a similar vein to Pat's work on create, drop etc, the db:charset
    task is now a one liner in databases.rake
This page is out of date. Refresh to see the latest.
View
15 activerecord/lib/active_record/railties/databases.rake
@@ -143,20 +143,7 @@ db_namespace = namespace :db do
# desc "Retrieves the charset for the current environment's database"
task :charset => [:environment, :load_config] do
- config = ActiveRecord::Base.configurations[Rails.env || 'development']
- case config['adapter']
- when /mysql/
- ActiveRecord::Base.establish_connection(config)
- puts ActiveRecord::Base.connection.charset
- when /postgresql/
- ActiveRecord::Base.establish_connection(config)
- puts ActiveRecord::Base.connection.encoding
- when /sqlite/
- ActiveRecord::Base.establish_connection(config)
- puts ActiveRecord::Base.connection.encoding
- else
- $stderr.puts 'sorry, your database adapter is not supported yet, feel free to submit a patch'
- end
+ puts ActiveRecord::Tasks::DatabaseTasks.charset_current
end
# desc "Retrieves the collation for the current environment's database"
View
9 activerecord/lib/active_record/tasks/database_tasks.rb
@@ -47,6 +47,15 @@ def drop_current(environment = Rails.env)
}
end
+ def charset_current(environment = Rails.env)
+ charset ActiveRecord::Base.configurations[environment]
+ end
+
+ def charset(*arguments)
+ configuration = arguments.first
+ class_for_adapter(configuration['adapter']).new(*arguments).charset
+ end
+
def purge(configuration)
class_for_adapter(configuration['adapter']).new(configuration).purge
end
View
6 activerecord/lib/active_record/tasks/mysql_database_tasks.rb
@@ -40,6 +40,10 @@ def purge
connection.recreate_database configuration['database'], creation_options
end
+ def charset
+ connection.charset
+ end
+
private
def configuration
@@ -90,4 +94,4 @@ def root_password
end
end
end
-end
+end
View
6 activerecord/lib/active_record/tasks/postgresql_database_tasks.rb
@@ -23,6 +23,10 @@ def drop
connection.drop_database configuration['database']
end
+ def charset
+ connection.encoding
+ end
+
def purge
clear_active_connections!
drop
@@ -47,4 +51,4 @@ def establish_master_connection
end
end
end
-end
+end
View
6 activerecord/lib/active_record/tasks/sqlite_database_tasks.rb
@@ -27,6 +27,10 @@ def drop
end
alias :purge :drop
+ def charset
+ connection.encoding
+ end
+
private
def configuration
@@ -38,4 +42,4 @@ def root
end
end
end
-end
+end
View
34 activerecord/test/cases/database_tasks_test.rb
@@ -294,4 +294,38 @@ def test_sqlite_create
ActiveRecord::Tasks::DatabaseTasks.purge 'adapter' => 'sqlite3'
end
end
+
+ class DatabaseTasksCharsetTest < ActiveRecord::TestCase
+ def setup
+ @mysql_tasks, @postgresql_tasks, @sqlite_tasks = stub, stub, stub
+ ActiveRecord::Tasks::MySQLDatabaseTasks.stubs(:new).returns @mysql_tasks
+ ActiveRecord::Tasks::PostgreSQLDatabaseTasks.stubs(:new).
+ returns @postgresql_tasks
+ ActiveRecord::Tasks::SQLiteDatabaseTasks.stubs(:new).returns @sqlite_tasks
+ end
+
+ def test_mysql_charset
+ @mysql_tasks.expects(:charset)
+
+ ActiveRecord::Tasks::DatabaseTasks.charset 'adapter' => 'mysql'
+ end
+
+ def test_mysql2_charset
+ @mysql_tasks.expects(:charset)
+
+ ActiveRecord::Tasks::DatabaseTasks.charset 'adapter' => 'mysql2'
+ end
+
+ def test_postgresql_charset
+ @postgresql_tasks.expects(:charset)
+
+ ActiveRecord::Tasks::DatabaseTasks.charset 'adapter' => 'postgresql'
+ end
+
+ def test_sqlite_charset
+ @sqlite_tasks.expects(:charset)
+
+ ActiveRecord::Tasks::DatabaseTasks.charset 'adapter' => 'sqlite3'
+ end
+ end
end
View
20 activerecord/test/cases/mysql_rake_test.rb
@@ -176,4 +176,22 @@ def test_recreates_database_with_the_given_options
)
end
end
-end
+
+ class MysqlDBCharsetTest < ActiveRecord::TestCase
+ def setup
+ @connection = stub(:create_database => true)
+ @configuration = {
+ 'adapter' => 'mysql',
+ 'database' => 'my-app-db'
+ }
+
+ ActiveRecord::Base.stubs(:connection).returns(@connection)
+ ActiveRecord::Base.stubs(:establish_connection).returns(true)
+ end
+
+ def test_db_retrieves_charset
+ @connection.expects(:charset)
+ ActiveRecord::Tasks::DatabaseTasks.charset @configuration
+ end
+ end

Just for consistency, could you move this and other db (sqlite/postgresql) charset tests to the end of the file instead? (like with DatabaseTasksCharsetTest). Also there's a typo in the class name: Charst => Charset.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
+end
View
20 activerecord/test/cases/postgresql_rake_test.rb
@@ -132,4 +132,22 @@ def test_establishes_connection
ActiveRecord::Tasks::DatabaseTasks.purge @configuration
end
end
-end
+
+ class PostgreSQLDBCharsetTest < ActiveRecord::TestCase
+ def setup
+ @connection = stub(:create_database => true)
+ @configuration = {
+ 'adapter' => 'postgresql',
+ 'database' => 'my-app-db'
+ }
+
+ ActiveRecord::Base.stubs(:connection).returns(@connection)
+ ActiveRecord::Base.stubs(:establish_connection).returns(true)
+ end
+
+ def test_db_retrieves_charset
+ @connection.expects(:encoding)
+ ActiveRecord::Tasks::DatabaseTasks.charset @configuration
+ end
+ end
+end
View
22 activerecord/test/cases/sqlite_rake_test.rb
@@ -103,4 +103,24 @@ def test_removes_file_with_relative_path
ActiveRecord::Tasks::DatabaseTasks.drop @configuration, '/rails/root'
end
end
-end
+
+ class SqliteDBCharsetTest < ActiveRecord::TestCase
+ def setup
+ @database = 'db_create.sqlite3'
+ @connection = stub :connection
+ @configuration = {
+ 'adapter' => 'sqlite3',
+ 'database' => @database
+ }
+
+ File.stubs(:exist?).returns(false)
+ ActiveRecord::Base.stubs(:connection).returns(@connection)
+ ActiveRecord::Base.stubs(:establish_connection).returns(true)
+ end
+
+ def test_db_retrieves_charset
+ @connection.expects(:encoding)
+ ActiveRecord::Tasks::DatabaseTasks.charset @configuration, '/rails/root'
+ end
+ end
+end
Something went wrong with that request. Please try again.