Move to --log-migrations-sql and --log-migrator-sql

Author: josevalim

lib/ecto/migration/runner.ex

@@ -343,14 +343,16 @@ defmodule Ecto.Migration.Runner do
     {:ok, logs} = repo.__adapter__().execute_ddl(meta, command, timeout: :infinity, log: sql)
 
     Enum.each(logs, fn {level, message, metadata} ->
-      Logger.log(level, message, metadata)
+      log(level, message, metadata)
     end)
 
     :ok
   end
 
-  defp log(false, _msg), do: :ok
-  defp log(level, msg),  do: Logger.log(level, msg)
+  defp log(level, msg, metadata \\ [])
+  defp log(false, _msg, _metadata), do: :ok
+  defp log(true, msg, metadata), do: Logger.log(:info, msg, metadata)
+  defp log(level, msg, metadata),  do: Logger.log(level, msg, metadata)
 
   defp command(ddl) when is_binary(ddl) or is_list(ddl),
     do: "execute #{inspect ddl}"

lib/ecto/migrator.ex

@@ -274,8 +274,8 @@ defmodule Ecto.Migrator do
     * `:log` - the level to use for logging of migration commands. Defaults to `:info`.
       Can be any of `Logger.level/0` values or a boolean.
     * `:log_migrations_sql` - the level to use for logging of SQL commands
-      generated by migrations. Defaults to `false`. Can be any of `Logger.level/0` values
-      or a boolean.
+      generated by migrations. Defaults to `false`. Can be any of `Logger.level/0`
+      values or a boolean.
     * `:log_migrator_sql` - the level to use for logging of SQL commands emitted
       by the migrator, such as transactions, locks, etc. Defaults to `false`.
       Can be any of `Logger.level/0` values or a boolean.
@@ -405,6 +405,14 @@ defmodule Ecto.Migrator do
   """
   @spec run(Ecto.Repo.t, String.t | [String.t] | [{integer, module}], atom, Keyword.t) :: [integer]
   def run(repo, migration_source, direction, opts) do
+    opts =
+      if log_sql = opts[:log_sql] do
+        IO.warn(":log_sql is deprecated, please use log_migrations_sql instead")
+        Keyword.put(opts, :log_migrations_sql, log_sql)
+      else
+        opts
+      end
+
     migration_source = List.wrap(migration_source)
 
     pending =

lib/mix/tasks/ecto.migrate.ex

@@ -18,7 +18,8 @@ defmodule Mix.Tasks.Ecto.Migrate do
     prefix: :string,
     pool_size: :integer,
     log_sql: :boolean,
-    log_sql_mode: :string,
+    log_migrations_sql: :boolean,
+    log_migrator_sql: :boolean,
     strict_version_order: :boolean,
     repo: [:keep, :string],
     no_compile: :boolean,
@@ -71,11 +72,10 @@ defmodule Mix.Tasks.Ecto.Migrate do
 
     * `--all` - run all pending migrations
 
-    * `--log-sql` - log the underlying sql statements for migrations
+    * `--log-migrations-sql` - log SQL generated by migration commands
 
-    * `--log-sql-mode` - how much SQL to log. `"migrations"` logs only the SQL
-      from commands in the migrations. `"all"` also includes transactions, table
-      locks, and so on. Defaults to `"migrations"`.
+    * `--log-migrator-sql` - log SQL generated by the migrator, such as
+      transactions, table locks, etc
 
     * `--migrations-path` - the path to load the migrations from, defaults to
       `"priv/repo/migrations"`. This option may be given multiple times in which
@@ -114,8 +114,10 @@ defmodule Mix.Tasks.Ecto.Migrate do
         do: opts,
         else: Keyword.put(opts, :all, true)
 
-    validate_log_sql_mode!(opts)
-    opts = conform_log_options(opts)
+    opts =
+      if opts[:quiet],
+        do: Keyword.merge(opts, [log: false, log_migrations_sql: false, log_migrator_sql: false]),
+        else: opts
 
     # Start ecto_sql explicitly before as we don't need
     # to restart those apps if migrated.
@@ -141,35 +143,4 @@ defmodule Mix.Tasks.Ecto.Migrate do
 
     :ok
   end
-
-  @doc false
-  def validate_log_sql_mode!(opts) do
-    case Keyword.get(opts, :log_sql_mode) do
-      nil -> :ok
-      "migrations" -> :ok
-      "all" -> :ok
-      mode ->
-        Mix.raise("""
-        #{inspect(mode)} is not a valid log_sql_mode.
-        Valid options are: "all", "migrations"
-        """)
-    end
-  end
-
-  @doc false
-  def conform_log_options(opts) do
-    opts =
-      if opts[:log_sql_mode] == "all",
-        do: Keyword.merge(opts, [log_migrations_sql: :info, log_migrator_sql: :info, log: :info]),
-        else: opts
-
-    opts =
-      if opts[:log_sql_mode] == "migrations",
-        do: Keyword.merge(opts, [log_migrations_sql: :info, log: :info]),
-        else: opts
-
-    if opts[:quiet],
-      do: Keyword.merge(opts, [log: false]),
-      else: opts
-  end
 end

lib/mix/tasks/ecto.rollback.ex

@@ -18,7 +18,8 @@ defmodule Mix.Tasks.Ecto.Rollback do
     prefix: :string,
     pool_size: :integer,
     log_sql: :boolean,
-    log_sql_mode: :string,
+    log_migrations_sql: :boolean,
+    log_migrator_sql: :boolean,
     repo: [:keep, :string],
     no_compile: :boolean,
     no_deps_check: :boolean,
@@ -67,11 +68,10 @@ defmodule Mix.Tasks.Ecto.Rollback do
 
     * `--all` - run all pending migrations
 
-    * `--log-sql` - log the underlying sql statements for migrations
+    * `--log-migrations-sql` - log SQL generated by migration commands
 
-    * `--log-sql-mode` - how much SQL to log. `"migrations"` logs only the SQL
-      from commands in the migrations. `"all"` also includes transactions, table
-      locks, and so on. Defaults to `"migrations"`.
+    * `--log-migrator-sql` - log SQL generated by the migrator, such as
+      transactions, table locks, etc
 
     * `--migrations-path` - the path to load the migrations from, defaults to
       `"priv/repo/migrations"`. This option may be given multiple times in which
@@ -103,16 +103,17 @@ defmodule Mix.Tasks.Ecto.Rollback do
   @impl true
   def run(args, migrator \\ &Ecto.Migrator.run/4) do
     repos = parse_repo(args)
-    {opts, _} = OptionParser.parse! args, strict: @switches, aliases: @aliases
+    {opts, _} = OptionParser.parse!(args, strict: @switches, aliases: @aliases)
 
     opts =
       if opts[:to] || opts[:step] || opts[:all],
         do: opts,
         else: Keyword.put(opts, :step, 1)
 
-    Mix.Tasks.Ecto.Migrate.validate_log_sql_mode!(opts)
-
-    opts = Mix.Tasks.Ecto.Migrate.conform_log_options(opts)
+    opts =
+      if opts[:quiet],
+        do: Keyword.merge(opts, [log: false, log_migrations_sql: false, log_migrator_sql: false]),
+        else: opts
 
     # Start ecto_sql explicitly before as we don't need
     # to restart those apps if migrated.

test/mix/tasks/ecto.migrate_test.exs

@@ -126,13 +126,6 @@ defmodule Mix.Tasks.Ecto.MigrateTest do
     assert !Process.get(:started)
   end
 
-  test "raises when invalid log_sql_mode is given" do
-    assert_raise Mix.Error, fn ->
-      run ["--log-sql-mode", "invalid"], fn _, _, _, _ -> [] end
-    end
-    assert !Process.get(:started)
-  end
-
   test "uses custom paths" do
     path1 = Path.join([unquote(tmp_path()), inspect(Ecto.Migrate), "migrations_1"])
     path2 = Path.join([unquote(tmp_path()), inspect(Ecto.Migrate), "migrations_2"])

test/mix/tasks/ecto.rollback_test.exs

@@ -90,13 +90,6 @@ defmodule Mix.Tasks.Ecto.RollbackTest do
     assert !Process.get(:started)
   end
 
-  test "raises when invalid log_sql_mode is given" do
-    assert_raise Mix.Error, fn ->
-      run ["--log-sql-mode", "invalid"], fn _, _, _, _ -> [] end
-    end
-    assert !Process.get(:started)
-  end
-
   test "uses custom paths" do
     path1 = Path.join([unquote(tmp_path()), inspect(Ecto.Migrate), "migrations_1"])
     path2 = Path.join([unquote(tmp_path()), inspect(Ecto.Migrate), "migrations_2"])