Repository: elixir-gettext/gettext
Pull request #287 (2021-09-16 01:29:38)

Added --check-unextracted to extract mix task
This PR addresses #286.

While still a work in progress (mostly test clean up that I want to do), I figured I would throw up the PR to get eyes on it sooner rather than later in case the approach I took was off.

Thanks in advance for reviewing the PR :)

juanperi: hey, this would be an awesome addition!
I have a question though. From the look at the PR, you are exactly matching the content of the previous file vs the new file, and failing if they don't.
Now, that is indeed correct, and very helpful. But what if there are no new translations, and the only thing that changed is the line in which a translation was taken from.
eg:
```
#, elixir-format
#: lib/app_web/templates/compare/index/_alternative_card.html.eex:49
msgid "# 1 Alternative"
msgstr ""
```
If we add a new line somewhere in that template, now the line will be 50, and your check will fail.

So, when should this test fail?:
1. If a new `gettext("content")` was added and it was not extracted
2. if any possible change (like line change) has happened and would make the POT file different
josevalim: @juanperi I think the goal is still raise, because it means someone forgot to run the command. My understanding is that the goal of the flag is basically "did someone remember to run mix getttext.extract before committing?". :) But I am interested in @akoutmos' take too.
akoutmos: I thought about this as well yesterday...and the conclusion that I came to was that while there are no new translations and the existing translations either changed lines/files, the POT file is still inaccurate with regards to the current state of the codebase. Hence why I made the PR the way that I did.
josevalim: Hrm, maybe we should change the name? After all it is checking more than unextracted. Maybe `--check-up-to-date` or `--check-updated`?
akoutmos: I guess `--check-unextracted` might be a bit misleading now that I think about it. I'd be ok with `--check-up-to-date` or perhaps `--check-current` for something shorter?
josevalim: Any is fine. I am glad to leave it up to you. :)
juanperi: hey, awesome we reached a consensus on the goal of the check. Thank you both.

I don't think that i would be able to use it for my particular case, where most of the time changes in the templates do not imply they are adding new translations, and although the line numbers are useful, they are mostly needed when new translations are made available. But I understand that strictness is better for a general check :)

Maybe the extra "configuration" flag for this check might be implemented later if you think it would be useful (i might work on that if there is interest)
akoutmos: Finished the last bit of test clean up so all set on my end :). 

Patch:
diff --git a/lib/mix/tasks/gettext.extract.ex b/lib/mix/tasks/gettext.extract.ex
index 78fc9ed..6a6d8bf 100644
--- a/lib/mix/tasks/gettext.extract.ex
+++ b/lib/mix/tasks/gettext.extract.ex
@@ -14,6 +14,13 @@ defmodule Mix.Tasks.Gettext.Extract do
   and `:priv` options given by Gettext modules when they call `use Gettext`. One
   POT file is generated for each translation domain.
 
+  If you would like to verify that your POT files are up to date with the
+  current state of the codebase, you can provide the `--check-up-to-date`
+  flag. Note that this validation will fail even when the same calls to gettext
+  only change location in the codebase:
+
+      mix gettext.extract --check-up-to-date
+
   It is possible to give the `--merge` option to perform merging
   for every Gettext backend updated during merge:
 
@@ -27,7 +34,7 @@ defmodule Mix.Tasks.Gettext.Extract do
 
   """
 
-  @switches [merge: :boolean]
+  @switches [merge: :boolean, check_up_to_date: :boolean]
 
   def run(args) do
     Application.ensure_all_started(:gettext)
@@ -36,6 +43,14 @@ defmodule Mix.Tasks.Gettext.Extract do
     {opts, _} = OptionParser.parse!(args, switches: @switches)
     pot_files = extract(mix_config[:app], mix_config[:gettext] || [])
 
+    if opts[:check_up_to_date] do
+      run_up_to_date_check(pot_files)
+    else
+      run_translation_extraction(pot_files, opts, args)
+    end
+  end
+
+  defp run_translation_extraction(pot_files, opts, args) do
     for {path, contents} <- pot_files do
       File.mkdir_p!(Path.dirname(path))
       File.write!(path, contents)
@@ -49,6 +64,41 @@ defmodule Mix.Tasks.Gettext.Extract do
     :ok
   end
 
+  defp run_up_to_date_check(pot_files) do
+    pot_files
+    |> Enum.reduce([], fn {path, contents}, not_extracted_acc ->
+      cond do
+        not File.exists?(path) ->
+          [path | not_extracted_acc]
+
+        not file_contents_match?(path, contents) ->
+          [path | not_extracted_acc]
+
+        true ->
+          not_extracted_acc
+      end
+    end)
+    |> check!()
+  end
+
+  defp file_contents_match?(path, contents) do
+    File.read!(path) == contents
+  end
+
+  defp check!([]), do: :ok
+
+  defp check!(not_extracted) do
+    Mix.raise("""
+    mix gettext.extract failed due to --check-up-to-date.
+    The following POT files were not extracted or are out of date:
+    #{to_bullet_list(not_extracted)}
+    """)
+  end
+
+  defp to_bullet_list(files) do
+    Enum.map_join(files, "\n", &"  * #{&1 |> to_string() |> Path.relative_to_cwd()}")
+  end
+
   defp extract(app, gettext_config) do
     Gettext.Extractor.enable()
     force_compile()


Test patch:
diff --git a/test/mix/tasks/gettext.extract_test.exs b/test/mix/tasks/gettext.extract_test.exs
index ad8c924..caccd35 100644
--- a/test/mix/tasks/gettext.extract_test.exs
+++ b/test/mix/tasks/gettext.extract_test.exs
@@ -5,25 +5,19 @@ defmodule Mix.Tasks.Gettext.ExtractTest do
 
   @priv_path "../../../tmp/gettext.extract" |> Path.expand(__DIR__) |> Path.relative_to_cwd()
 
+  setup_all do
+    # To suppress the `redefining module MyApp` warnings for the test modules
+    Code.compiler_options(ignore_module_conflict: true)
+    :ok
+  end
+
   setup do
     File.rm_rf!(@priv_path)
     :ok
   end
 
   test "extracting and extracting with --merge" do
-    write_file("mix.exs", """
-    defmodule MyApp.MixProject do
-      use Mix.Project
-
-      def project() do
-        [app: :my_app, version: "0.1.0"]
-      end
-
-      def application() do
-        [extra_applications: [:logger, :gettext]]
-      end
-    end
-    """)
+    create_test_mix_file()
 
     write_file("lib/my_app.ex", """
     defmodule MyApp.Gettext do
@@ -73,6 +67,110 @@ defmodule Mix.Tasks.Gettext.ExtractTest do
            """
   end
 
+  test "--check-up-to-date should fail if no POT files have been created" do
+    create_test_mix_file()
+
+    write_file("lib/my_app.ex", """
+    defmodule MyApp.Gettext do
+    use Gettext, otp_app: :my_app
+    end
+
+    defmodule MyApp do
+    require MyApp.Gettext
+    def foo(), do: MyApp.Gettext.gettext("hello")
+    end
+    """)
+
+    write_file("lib/other.ex", """
+    defmodule MyApp.Other do
+      require MyApp.Gettext
+      def foo(), do: MyApp.Gettext.dgettext("my_domain", "other")
+    end
+    """)
+
+    expected_message = """
+    mix gettext.extract failed due to --check-up-to-date.
+    The following POT files were not extracted or are out of date:
+      * priv/gettext/my_domain.pot
+      * priv/gettext/default.pot
+    """
+
+    capture_io(fn ->
+      assert_raise Mix.Error, expected_message, fn ->
+        Mix.Project.in_project(:my_app, tmp_path("/"), fn _module ->
+          run(["--check-up-to-date"])
+        end)
+      end
+    end)
+  end
+
+  test "--check-up-to-date should fail if POT files are outdated" do
+    create_test_mix_file()
+
+    write_file("lib/my_app.ex", """
+    defmodule MyApp.Gettext do
+    use Gettext, otp_app: :my_app
+    end
+
+    defmodule MyApp do
+    require MyApp.Gettext
+    def foo(), do: MyApp.Gettext.gettext("hello")
+    end
+    """)
+
+    write_file("lib/other.ex", """
+    defmodule MyApp.Other do
+      require MyApp.Gettext
+      def foo(), do: MyApp.Gettext.dgettext("my_domain", "other")
+    end
+    """)
+
+    capture_io(fn ->
+      Mix.Project.in_project(:my_app, tmp_path("/"), fn _module -> run([]) end)
+    end)
+
+    write_file("lib/my_app.ex", """
+    defmodule MyApp.Gettext do
+    use Gettext, otp_app: :my_app
+    end
+
+    defmodule MyApp do
+    require MyApp.Gettext
+    def foo(), do: MyApp.Gettext.gettext("new text")
+    end
+    """)
+
+    expected_message = """
+    mix gettext.extract failed due to --check-up-to-date.
+    The following POT files were not extracted or are out of date:
+      * priv/gettext/default.pot
+    """
+
+    capture_io(fn ->
+      assert_raise Mix.Error, expected_message, fn ->
+        Mix.Project.in_project(:my_app, tmp_path("/"), fn _module ->
+          run(["--check-up-to-date"])
+        end)
+      end
+    end)
+  end
+
+  defp create_test_mix_file do
+    write_file("mix.exs", """
+    defmodule MyApp.MixProject do
+      use Mix.Project
+
+      def project() do
+        [app: :my_app, version: "0.1.0"]
+      end
+
+      def application() do
+        [extra_applications: [:logger, :gettext]]
+      end
+    end
+    """)
+  end
+
   defp write_file(path, contents) do
     path = tmp_path(path)
     File.mkdir_p!(Path.dirname(path))