Repository: jeremyjh/dialyxir
Pull request #463 (2022-07-12 20:00:09)

feat: Adds a github formatter
I would am working to create a github actions formatter for this library, and the first step I wanted to take is to extract the other formats into modules.

jeremyjh: Hi, I'm not opposed to the refactoring but can you explain a bit more about what you are trying to accomplish? Is it returning the results in a structure understood by github actions so they appear better in PRs? And then you'll do another PR to implement an action formatter?
isaacsanders: https://docs.github.com/en/actions/using-workflows/workflow-commands-for-github-actions#setting-an-error-message

I have the action formatter written already, but I figured doing changes in smaller chunks was a better approach.

The link above outlines the format. It allows for inline annotations of the files from the github check run, i believe.
isaacsanders: Added in. Thanks for the review and promptness!
isaacsanders: I've added the docs
jeremyjh: @isaacsanders I just realized I had to release it to run CI, there are a couple of compile warnings can you please check?


Patch:
diff --git a/README.md b/README.md
index 752fda2..21ce7dc 100644
--- a/README.md
+++ b/README.md
@@ -49,14 +49,16 @@ mix dialyzer
 
 ### Command line options
 
-  * `--no-compile`         - do not compile even if needed.
-  * `--no-check`           - do not perform (quick) check to see if PLT needs to be updated.
-  * `--ignore-exit-status` - display warnings but do not halt the VM or return an exit status code.
-  *  `--format short`      - format the warnings in a compact format, suitable for ignore file using Elixir term format.
-  *  `--format raw`        - format the warnings in format returned before Dialyzer formatting.
-  *  `--format dialyxir`   - format the warnings in a pretty printed format.
-  *  `--format dialyzer`   - format the warnings in the original Dialyzer format, suitable for ignore file using simple string matches.
-  *  `--quiet`             - suppress all informational messages.
+  * `--no-compile`          - do not compile even if needed.
+  * `--no-check`            - do not perform (quick) check to see if PLT needs to be updated.
+  * `--ignore-exit-status`  - display warnings but do not halt the VM or return an exit status code.
+  *  `--format short`       - format the warnings in a compact format, suitable for ignore file using Elixir term format.
+  *  `--format raw`         - format the warnings in format returned before Dialyzer formatting.
+  *  `--format dialyxir`    - format the warnings in a pretty printed format.
+  *  `--format dialyzer`    - format the warnings in the original Dialyzer format, suitable for ignore file using simple string matches.
+  *  `--format github`      - format the warnings in the Github Actions message format.
+  *  `--format ignore_file` - format the warnings to be suitable for adding to Elixir Format ignore file.
+  *  `--quiet`              - suppress all informational messages.
 
 Warning flags passed to this task are passed on to `:dialyzer` - e.g.
 
@@ -133,7 +135,7 @@ cache:
         run: mix dialyzer --plt
 
       - name: Run dialyzer
-        run: mix dialyzer
+        run: mix dialyzer --format github
 
 ```
 
diff --git a/lib/dialyxir/dialyzer.ex b/lib/dialyxir/dialyzer.ex
index c68ee89..c3374e5 100644
--- a/lib/dialyxir/dialyzer.ex
+++ b/lib/dialyxir/dialyzer.ex
@@ -20,25 +20,28 @@ defmodule Dialyxir.Dialyzer do
         formatter =
           cond do
             split[:format] == "dialyzer" ->
-              :dialyzer
+              Dialyxir.Formatter.Dialyzer
 
             split[:format] == "dialyxir" ->
-              :dialyxir
+              Dialyxir.Formatter.Dialyxir
+
+            split[:format] == "github" ->
+              Dialyxir.Formatter.Github
 
             split[:format] == "ignore_file" ->
-              :ignore_file
+              Dialyxir.Formatter.IgnoreFile
 
             split[:format] == "raw" ->
-              :raw
+              Dialyxir.Formatter.Raw
 
             split[:format] == "short" ->
-              :short
+              Dialyxir.Formatter.Short
 
             split[:raw] ->
-              :raw
+              Dialyxir.Formatter.Raw
 
             true ->
-              :dialyxir
+              Dialyxir.Formatter.Dialyxir
           end
 
         info("Starting Dialyzer")
diff --git a/lib/dialyxir/formatter.ex b/lib/dialyxir/formatter.ex
index 29744cd..ed97eb7 100644
--- a/lib/dialyxir/formatter.ex
+++ b/lib/dialyxir/formatter.ex
@@ -9,13 +9,19 @@ defmodule Dialyxir.Formatter do
 
   alias Dialyxir.FilterMap
 
+  @type warning() :: {tag :: term(), {file :: Path.t(), line :: pos_integer()}, {atom(), list()}}
+
+  @type t() :: module()
+
+  @callback format(warning()) :: String.t()
+
   def formatted_time(duration_us) do
     minutes = div(duration_us, 60_000_000)
     seconds = (rem(duration_us, 60_000_000) / 1_000_000) |> Float.round(2)
     "done in #{minutes}m#{seconds}s"
   end
 
-  @spec format_and_filter([tuple], module, Keyword.t(), atom) :: tuple
+  @spec format_and_filter([tuple], module, Keyword.t(), t()) :: tuple
   def format_and_filter(warnings, filterer, filter_map_args, formatter) do
     filter_map = filterer.filter_map(filter_map_args)
 
@@ -24,7 +30,7 @@ defmodule Dialyxir.Formatter do
     formatted_warnings =
       filtered_warnings
       |> filter_legacy_warnings(filterer)
-      |> Enum.map(&format_warning(&1, formatter))
+      |> Enum.map(&formatter.format/1)
       |> Enum.uniq()
 
     show_count_skipped(warnings, formatted_warnings, filter_map)
@@ -46,114 +52,6 @@ defmodule Dialyxir.Formatter do
     end
   end
 
-  defp format_warning(warning, :raw) do
-    inspect(warning, limit: :infinity)
-  end
-
-  defp format_warning(warning, :dialyzer) do
-    # OTP 22 uses indented output, but that's incompatible with dialyzer.ignore-warnings format.
-    # Can be disabled, but OTP 21 and older only accept an atom, so only disable on OTP 22+.
-    opts =
-      if String.to_integer(System.otp_release()) < 22,
-        do: :fullpath,
-        else: [{:filename_opt, :fullpath}, {:indent_opt, false}]
-
-    warning
-    |> :dialyzer.format_warning(opts)
-    |> String.Chars.to_string()
-    |> String.replace_trailing("\n", "")
-  end
-
-  defp format_warning({_tag, {file, line}, message}, :short) do
-    {warning_name, arguments} = message
-    base_name = Path.relative_to_cwd(file)
-
-    warning = warning(warning_name)
-    string = warning.format_short(arguments)
-
-    "#{base_name}:#{line}:#{warning_name} #{string}"
-  end
-
-  defp format_warning({_tag, {file, _line}, {warning_name, _arguments}}, :ignore_file) do
-    ~s({"#{file}", :#{warning_name}},)
-  end
-
-  defp format_warning(dialyzer_warning = {_tag, {file, line}, message}, :dialyxir) do
-    {warning_name, arguments} = message
-    base_name = Path.relative_to_cwd(file)
-
-    formatted =
-      try do
-        warning = warning(warning_name)
-        string = warning.format_long(arguments)
-
-        """
-        #{base_name}:#{line}:#{warning_name}
-        #{string}
-        """
-      rescue
-        e ->
-          message = """
-          Unknown error occurred: #{inspect(e)}
-          """
-
-          wrap_error_message(message, dialyzer_warning)
-      catch
-        {:error, :unknown_warning, warning_name} ->
-          message = """
-          Unknown warning:
-          #{inspect(warning_name)}
-          """
-
-          wrap_error_message(message, dialyzer_warning)
-
-        {:error, :lexing, warning} ->
-          message = """
-          Failed to lex warning:
-          #{inspect(warning)}
-          """
-
-          wrap_error_message(message, dialyzer_warning)
-
-        {:error, :parsing, failing_string} ->
-          message = """
-          Failed to parse warning:
-          #{inspect(failing_string)}
-          """
-
-          wrap_error_message(message, dialyzer_warning)
-
-        {:error, :pretty_printing, failing_string} ->
-          message = """
-          Failed to pretty print warning:
-          #{inspect(failing_string)}
-          """
-
-          wrap_error_message(message, dialyzer_warning)
-
-        {:error, :formatting, code} ->
-          message = """
-          Failed to format warning:
-          #{inspect(code)}
-          """
-
-          wrap_error_message(message, dialyzer_warning)
-      end
-
-    formatted <> String.duplicate("_", 80)
-  end
-
-  defp wrap_error_message(message, warning) do
-    """
-    Please file a bug in https://github.com/jeremyjh/dialyxir/issues with this message.
-
-    #{message}
-
-    Legacy warning:
-    #{format_warning(warning, :dialyzer)}
-    """
-  end
-
   defp show_count_skipped(warnings, filtered_warnings, filter_map) do
     warnings_count = Enum.count(warnings)
     filtered_warnings_count = Enum.count(filtered_warnings)
@@ -189,16 +87,6 @@ defmodule Dialyxir.Formatter do
     |> Enum.count()
   end
 
-  defp warning(warning_name) do
-    warnings = Dialyxir.Warnings.warnings()
-
-    if Map.has_key?(warnings, warning_name) do
-      Map.get(warnings, warning_name)
-    else
-      throw({:error, :unknown_warning, warning_name})
-    end
-  end
-
   defp filter_warnings(warnings, filterer, filter_map) do
     {warnings, filter_map} =
       Enum.map_reduce(warnings, filter_map, &filter_warning(filterer, &1, &2))
@@ -212,7 +100,7 @@ defmodule Dialyxir.Formatter do
       {skip?, matching_filters} =
         try do
           filterer.filter_warning?(
-            {to_string(file), warning_type, line, format_warning(warning, :short)},
+            {to_string(file), warning_type, line, Dialyxir.Formatter.Short.format(warning)},
             filter_map
           )
         rescue
@@ -242,7 +130,7 @@ defmodule Dialyxir.Formatter do
     Enum.reject(warnings, fn warning ->
       formatted_warnings =
         warning
-        |> format_warning(:dialyzer)
+        |> Dialyxir.Formatter.Dialyzer.format()
         |> List.wrap()
 
       Enum.empty?(filterer.filter_legacy_warnings(formatted_warnings))
diff --git a/lib/dialyxir/formatter/dialyxir.ex b/lib/dialyxir/formatter/dialyxir.ex
new file mode 100644
index 0000000..84651ad
--- /dev/null
+++ b/lib/dialyxir/formatter/dialyxir.ex
@@ -0,0 +1,92 @@
+defmodule Dialyxir.Formatter.Dialyxir do
+  @moduledoc false
+
+  @behaviour Dialyxir.Formatter
+
+  @impl Dialyxir.Formatter
+  def format(dialyzer_warning = {_tag, {file, line}, message}) do
+    {warning_name, arguments} = message
+    base_name = Path.relative_to_cwd(file)
+
+    formatted =
+      try do
+        warning = warning(warning_name)
+        string = warning.format_long(arguments)
+
+        """
+        #{base_name}:#{line}:#{warning_name}
+        #{string}
+        """
+      rescue
+        e ->
+          message = """
+          Unknown error occurred: #{inspect(e)}
+          """
+
+          wrap_error_message(message, dialyzer_warning)
+      catch
+        {:error, :unknown_warning, warning_name} ->
+          message = """
+          Unknown warning:
+          #{inspect(warning_name)}
+          """
+
+          wrap_error_message(message, dialyzer_warning)
+
+        {:error, :lexing, warning} ->
+          message = """
+          Failed to lex warning:
+          #{inspect(warning)}
+          """
+
+          wrap_error_message(message, dialyzer_warning)
+
+        {:error, :parsing, failing_string} ->
+          message = """
+          Failed to parse warning:
+          #{inspect(failing_string)}
+          """
+
+          wrap_error_message(message, dialyzer_warning)
+
+        {:error, :pretty_printing, failing_string} ->
+          message = """
+          Failed to pretty print warning:
+          #{inspect(failing_string)}
+          """
+
+          wrap_error_message(message, dialyzer_warning)
+
+        {:error, :formatting, code} ->
+          message = """
+          Failed to format warning:
+          #{inspect(code)}
+          """
+
+          wrap_error_message(message, dialyzer_warning)
+      end
+
+    formatted <> String.duplicate("_", 80)
+  end
+
+  defp wrap_error_message(message, warning) do
+    """
+    Please file a bug in https://github.com/jeremyjh/dialyxir/issues with this message.
+
+    #{message}
+
+    Legacy warning:
+    #{Dialyxir.Formatter.Dialyzer.format(warning)}
+    """
+  end
+
+  defp warning(warning_name) do
+    warnings = Dialyxir.Warnings.warnings()
+
+    if Map.has_key?(warnings, warning_name) do
+      Map.get(warnings, warning_name)
+    else
+      throw({:error, :unknown_warning, warning_name})
+    end
+  end
+end
diff --git a/lib/dialyxir/formatter/dialyzer.ex b/lib/dialyxir/formatter/dialyzer.ex
new file mode 100644
index 0000000..934eabd
--- /dev/null
+++ b/lib/dialyxir/formatter/dialyzer.ex
@@ -0,0 +1,20 @@
+defmodule Dialyxir.Formatter.Dialyzer do
+  @moduledoc false
+
+  @behaviour Dialyxir.Formatter
+
+  @impl Dialyxir.Formatter
+  def format(warning) do
+    # OTP 22 uses indented output, but that's incompatible with dialyzer.ignore-warnings format.
+    # Can be disabled, but OTP 21 and older only accept an atom, so only disable on OTP 22+.
+    opts =
+      if String.to_integer(System.otp_release()) < 22,
+        do: :fullpath,
+        else: [{:filename_opt, :fullpath}, {:indent_opt, false}]
+
+    warning
+    |> :dialyzer.format_warning(opts)
+    |> String.Chars.to_string()
+    |> String.replace_trailing("\n", "")
+  end
+end
diff --git a/lib/dialyxir/formatter/github.ex b/lib/dialyxir/formatter/github.ex
new file mode 100644
index 0000000..999cbcd
--- /dev/null
+++ b/lib/dialyxir/formatter/github.ex
@@ -0,0 +1,25 @@
+defmodule Dialyxir.Formatter.Github do
+  @moduledoc false
+
+  @behaviour Dialyxir.Formatter
+
+  @impl Dialyxir.Formatter
+  def format({_tag, {file, line}, {warning_name, arguments}}) do
+    base_name = Path.relative_to_cwd(file)
+
+    warning = warning(warning_name)
+    string = warning.format_short(arguments)
+
+    "::warning file=#{base_name},line=#{line},title=#{warning_name}::#{string}"
+  end
+
+  defp warning(warning_name) do
+    warnings = Dialyxir.Warnings.warnings()
+
+    if Map.has_key?(warnings, warning_name) do
+      Map.get(warnings, warning_name)
+    else
+      throw({:error, :unknown_warning, warning_name})
+    end
+  end
+end
diff --git a/lib/dialyxir/formatter/ignore_file.ex b/lib/dialyxir/formatter/ignore_file.ex
new file mode 100644
index 0000000..b1bf6cc
--- /dev/null
+++ b/lib/dialyxir/formatter/ignore_file.ex
@@ -0,0 +1,10 @@
+defmodule Dialyxir.Formatter.IgnoreFile do
+  @moduledoc false
+
+  @behaviour Dialyxir.Formatter
+
+  @impl Dialyxir.Formatter
+  def format({_tag, {file, _line}, {warning_name, _arguments}}) do
+    ~s({"#{file}", :#{warning_name}},)
+  end
+end
diff --git a/lib/dialyxir/formatter/raw.ex b/lib/dialyxir/formatter/raw.ex
new file mode 100644
index 0000000..c18a221
--- /dev/null
+++ b/lib/dialyxir/formatter/raw.ex
@@ -0,0 +1,10 @@
+defmodule Dialyxir.Formatter.Raw do
+  @moduledoc false
+
+  @behaviour Dialyxir.Formatter
+
+  @impl Dialyxir.Formatter
+  def format(warning) do
+    inspect(warning, limit: :infinity)
+  end
+end
diff --git a/lib/dialyxir/formatter/short.ex b/lib/dialyxir/formatter/short.ex
new file mode 100644
index 0000000..dbb32de
--- /dev/null
+++ b/lib/dialyxir/formatter/short.ex
@@ -0,0 +1,25 @@
+defmodule Dialyxir.Formatter.Short do
+  @moduledoc false
+
+  @behaviour Dialyxir.Formatter
+
+  @impl Dialyxir.Formatter
+  def format({_tag, {file, line}, {warning_name, arguments}}) do
+    base_name = Path.relative_to_cwd(file)
+
+    warning = warning(warning_name)
+    string = warning.format_short(arguments)
+
+    "#{base_name}:#{line}:#{warning_name} #{string}"
+  end
+
+  defp warning(warning_name) do
+    warnings = Dialyxir.Warnings.warnings()
+
+    if Map.has_key?(warnings, warning_name) do
+      Map.get(warnings, warning_name)
+    else
+      throw({:error, :unknown_warning, warning_name})
+    end
+  end
+end
diff --git a/lib/mix/tasks/dialyzer.ex b/lib/mix/tasks/dialyzer.ex
index d243ea9..9d240ab 100644
--- a/lib/mix/tasks/dialyzer.ex
+++ b/lib/mix/tasks/dialyzer.ex
@@ -17,10 +17,11 @@ defmodule Mix.Tasks.Dialyzer do
     * `--list-unused-filters` - list unused ignore filters useful for CI. do
       not use with `mix do`.
     * `--plt` - only build the required PLT(s) and exit
-    * `--format short` - format the warnings in a compact format
-    * `--format raw` - format the warnings in format returned before Dialyzer formatting
-    * `--format dialyxir` - format the warnings in a pretty printed format
-    * `--format dialyzer` - format the warnings in the original Dialyzer format
+    * `--format short`       - format the warnings in a compact format
+    * `--format raw`         - format the warnings in format returned before Dialyzer formatting
+    * `--format dialyxir`    - format the warnings in a pretty printed format
+    * `--format dialyzer`    - format the warnings in the original Dialyzer format
+    * `--format github`      - format the warnings in the Github Actions message format
     * `--format ignore_file` - format the warnings to be suitable for adding to Elixir Format ignore file
     * `--quiet` - suppress all informational messages
 


Test patch:
diff --git a/test/dialyxir/formatter_test.exs b/test/dialyxir/formatter_test.exs
index 235993a..e9e4f31 100644
--- a/test/dialyxir/formatter_test.exs
+++ b/test/dialyxir/formatter_test.exs
@@ -4,6 +4,9 @@ defmodule Dialyxir.FormatterTest do
   import ExUnit.CaptureIO, only: [capture_io: 1]
 
   alias Dialyxir.Formatter
+  alias Dialyxir.Formatter.Dialyxir, as: DialyxirFormatter
+  alias Dialyxir.Formatter.Dialyzer, as: DialyzerFormatter
+  alias Dialyxir.Formatter.Short, as: ShortFormatter
   alias Dialyxir.Project
 
   defp in_project(app, f) when is_atom(app) do
@@ -23,7 +26,7 @@ defmodule Dialyxir.FormatterTest do
 
       in_project(:ignore, fn ->
         {:error, remaining, _unused_filters_present} =
-          Formatter.format_and_filter(warnings, Project, [], :short)
+          Formatter.format_and_filter(warnings, Project, [], ShortFormatter)
 
         assert remaining == []
       end)
@@ -35,7 +38,9 @@ defmodule Dialyxir.FormatterTest do
          {:no_return, [:only_normal, :format_long, 1]}}
 
       in_project(:ignore, fn ->
-        {:error, [remaining], _} = Formatter.format_and_filter([warning], Project, [], :short)
+        {:error, [remaining], _} =
+          Formatter.format_and_filter([warning], Project, [], ShortFormatter)
+
         assert remaining =~ ~r/different_file.* no local return/
       end)
     end
@@ -47,7 +52,7 @@ defmodule Dialyxir.FormatterTest do
 
       in_project(:ignore, fn ->
         {:error, remaining, _unused_filters_present} =
-          Formatter.format_and_filter([warning], Project, [], :short)
+          Formatter.format_and_filter([warning], Project, [], ShortFormatter)
 
         assert remaining == []
       end)
@@ -127,7 +132,7 @@ defmodule Dialyxir.FormatterTest do
 
     filter_args = [{:list_unused_filters, true}]
 
-    for format <- [:short, :dialyxir, :dialyzer] do
+    for format <- [ShortFormatter, DialyxirFormatter, DialyzerFormatter] do
       in_project(:ignore, fn ->
         capture_io(fn ->
           result = Formatter.format_and_filter(warnings, Project, filter_args, format)