Repository: adobe/elixir-styler
Pull request #148 (2024-04-02 15:49:47)

organize config files
It would be possible to ship this rule as a lint-only, which would assert that when sorted the config file doesn't change. That would unblock it from the following todos:

TODOs

- [x] treat comments like we do when moving module directives
- [x] add an empty line between stanzas for new apps (ala the newline from alias stanzas -> require stanzas
- [x] handle `=` assignments via either linking them to the following `config` statement or moving them to the top of the file


Patch:
diff --git a/CHANGELOG.md b/CHANGELOG.md
index 535de59..ac66931 100644
--- a/CHANGELOG.md
+++ b/CHANGELOG.md
@@ -10,6 +10,7 @@
 * `@derive`: move `@derive` before `defstruct|schema|embedded_schema` declarations (fixes compiler warning!) #134
 * strings: rewrite double-quoted strings to use `~s` when there's 4+ escaped double-quotes
   (`"\"\"\"\""` -> `~s("""")`) (`Credo.Check.Readability.StringSigils`) #146
+* `config/*.exs` files: organize `config app, ...` stanzas. this is another big one!
 
 ### Fixes
 
diff --git a/lib/style.ex b/lib/style.ex
index f6aaf9e..43a64b8 100644
--- a/lib/style.ex
+++ b/lib/style.ex
@@ -64,6 +64,9 @@ defmodule Styler.Style do
     |> Zipper.root()
   end
 
+  # useful for comparing AST without meta (line numbers, etc) interfering
+  def without_meta(ast), do: update_all_meta(ast, fn _ -> nil end)
+
   @doc """
   Returns the current node (wrapped in a `__block__` if necessary) if it's a valid place to insert additional nodes
   """
@@ -153,4 +156,97 @@ defmodule Styler.Style do
     end)
     |> Enum.sort_by(& &1.line)
   end
+
+  @doc """
+  Takes a list of nodes and clumps them up, setting `end_of_expression: [newlines: x]` to 1 for all but the final node,
+  which gets 2 instead, (hopefully!) creating an empty line before whatever follows.
+  """
+  def reset_newlines([]), do: []
+  def reset_newlines(nodes), do: reset_newlines(nodes, [])
+
+  def reset_newlines([node], acc), do: Enum.reverse([set_newlines(node, 2) | acc])
+  def reset_newlines([node | nodes], acc), do: reset_newlines(nodes, [set_newlines(node, 1) | acc])
+
+  defp set_newlines({directive, meta, children}, newline) do
+    updated_meta = Keyword.update(meta, :end_of_expression, [newlines: newline], &Keyword.put(&1, :newlines, newline))
+    {directive, updated_meta, children}
+  end
+
+  @doc """
+  "Fixes" the line numbers of nodes who have had their orders changed via sorting or other methods.
+  This "fix" siply ensures that comments don't get wrecked as part of us moving AST nodes willy-nilly.
+
+  The fix is rather naive, and simply enforces the following property on the code:
+  A given node must have a line number less than the following node.
+  Et voila! Comments behave much better.
+
+  ## In Detail
+
+  For example, given document
+
+    1: defmodule ...
+    2: alias B
+    3: # this is foo
+    4: def foo ...
+    5: alias A
+
+  Sorting aliases the ast node for  would put `alias A` (line 5) before `alias B` (line 2).
+
+    1: defmodule ...
+    5: alias A
+    2: alias B
+    3: # this is foo
+    4: def foo ...
+
+  Elixir's document algebra would then encounter `line: 5` and immediately dump all comments with `line <= 5`,
+  meaning after running through the formatter we'd end up with
+
+    1: defmodule
+    2: # hi
+    3: # this is foo
+    4: alias A
+    5: alias B
+    6:
+    7: def foo ...
+
+  This function fixes that by seeing that `alias A` has a higher line number than its following sibling `alias B` and so
+  updates `alias A`'s line to be preceding `alias B`'s line.
+
+  Running the results of this function through the formatter now no longer dumps the comments prematurely
+
+    1: defmodule ...
+    2: alias A
+    3: alias B
+    4: # this is foo
+    5: def foo ...
+  """
+  def fix_line_numbers(to_fix, acc \\ [], first_normal_line)
+
+  def fix_line_numbers([this, next | rest], acc, first_non_directive) do
+    this = cap_line(this, next)
+    fix_line_numbers([next | rest], [this | acc], first_non_directive)
+  end
+
+  def fix_line_numbers([last], acc, first_non_directive) do
+    last = if first_non_directive, do: cap_line(last, first_non_directive), else: last
+    Enum.reverse([last | acc])
+  end
+
+  def fix_line_numbers([], [], _), do: []
+
+  defp cap_line({_, this_meta, _} = this, {_, next_meta, _}) do
+    this_line = this_meta[:line]
+    next_line = next_meta[:line]
+
+    if this_line > next_line do
+      # Subtracting 2 helps the behaviour with one-liner comments preceding the next node. It's a bit of a hack.
+      # TODO: look into the comments list and
+      # 1. move comment blocks preceding `this` up with it
+      # 2. find the earliest comment before `next` and set `new_line` to that value - 1
+      desired_line = next_line - 2
+      shift_line(this, desired_line - this_line)
+    else
+      this
+    end
+  end
 end
diff --git a/lib/style/blocks.ex b/lib/style/blocks.ex
index 54a4511..1f3ac71 100644
--- a/lib/style/blocks.ex
+++ b/lib/style/blocks.ex
@@ -254,10 +254,7 @@ defmodule Styler.Style.Blocks do
   defp left_arrow?({:<-, _, _}), do: true
   defp left_arrow?(_), do: false
 
-  defp nodes_equivalent?(a, b) do
-    # compare nodes without metadata
-    Style.update_all_meta(a, fn _ -> nil end) == Style.update_all_meta(b, fn _ -> nil end)
-  end
+  defp nodes_equivalent?(a, b), do: Style.without_meta(a) == Style.without_meta(b)
 
   defp if_ast(zipper, head, {_, _, _} = do_body, {_, _, _} = else_body, ctx) do
     do_ = {{:__block__, [line: nil], [:do]}, do_body}
diff --git a/lib/style/configs.ex b/lib/style/configs.ex
new file mode 100644
index 0000000..5a7f4bc
--- /dev/null
+++ b/lib/style/configs.ex
@@ -0,0 +1,91 @@
+# Copyright 2024 Adobe. All rights reserved.
+# This file is licensed to you under the Apache License, Version 2.0 (the "License");
+# you may not use this file except in compliance with the License. You may obtain a copy
+# of the License at http://www.apache.org/licenses/LICENSE-2.0
+
+# Unless required by applicable law or agreed to in writing, software distributed under
+# the License is distributed on an "AS IS" BASIS, WITHOUT WARRANTIES OR REPRESENTATIONS
+# OF ANY KIND, either express or implied. See the License for the specific language
+# governing permissions and limitations under the License.
+
+defmodule Styler.Style.Configs do
+  @moduledoc """
+  Orders `Config.config/2,3` stanzas in configuration files.
+
+  - ordering is done only within immediate-sibling config statements
+  - assignments are moved above the configuration blocks
+  - any non `config/2,3` or assignment (`=/2`) calls mark the end of a sorting block.
+    this is support having conditional blocks (`if/case/cond`) and `import_config` stanzas between blocks
+
+  ### Breakages
+
+  If you configure the same values multiple times, Styler may swap their orders
+
+  **Before**
+
+    line 04: config :foo, bar: :zab
+    line 40: config :foo, bar: :baz
+
+    # Application.fetch_env!(:foo)[:bar] => :baz
+
+  **After**
+
+    line 04: config :foo, bar: :baz
+    line 05: config :foo, bar: :zab
+
+    # Application.fetch_env!(:foo)[:bar] => :zab
+
+  **Fix**
+
+  The reason Styler sorts configuration is to help you noticed these duplicated configuration stanzas.
+  Delete the duplicative/erroneous stanza and life will be good.
+  """
+
+  alias Styler.Style
+
+  def run({{:import, _, [{:__aliases__, _, [:Config]}]}, _} = zipper, %{config?: true} = ctx) do
+    {:skip, zipper, Map.put(ctx, :mix_config?, true)}
+  end
+
+  def run({{:config, _, [_, _ | _]} = config, zm}, %{mix_config?: true} = ctx) do
+    # all of these list are reversed due to the reduce
+    {configs, assignments, rest} =
+      Enum.reduce(zm.r, {[], [], []}, fn
+        {:config, _, [_, _ | _]} = config, {configs, assignments, []} -> {[config | configs], assignments, []}
+        {:=, _, [_lhs, _rhs]} = assignment, {configs, assignments, []} -> {configs, [assignment | assignments], []}
+        other, {configs, assignments, rest} -> {configs, assignments, [other | rest]}
+      end)
+
+    [config | configs] =
+      [config | configs]
+      |> Enum.group_by(fn
+        {:config, _, [{:__block__, _, [app]} | _]} -> app
+        {:config, _, [arg | _]} -> Style.without_meta(arg)
+      end)
+      |> Enum.sort(:desc)
+      |> Enum.flat_map(fn {_app, configs} ->
+        configs
+        |> Enum.sort_by(&Style.without_meta/1, :asc)
+        |> Style.reset_newlines()
+        |> Enum.reverse()
+      end)
+      |> Style.fix_line_numbers(List.first(rest))
+
+    assignments = assignments |> Enum.reverse() |> Style.reset_newlines()
+
+    zm = %{zm | l: configs ++ Enum.reverse(assignments, zm.l), r: Enum.reverse(rest)}
+    {:skip, {config, zm}, ctx}
+  end
+
+  def run(zipper, %{config?: true} = ctx) do
+    {:cont, zipper, ctx}
+  end
+
+  def run(zipper, %{file: file} = ctx) do
+    if file =~ ~r|config/.*\.exs| do
+      {:cont, zipper, Map.put(ctx, :config?, true)}
+    else
+      {:halt, zipper, ctx}
+    end
+  end
+end
diff --git a/lib/style/module_directives.ex b/lib/style/module_directives.ex
index eb4d13d..5588010 100644
--- a/lib/style/module_directives.ex
+++ b/lib/style/module_directives.ex
@@ -196,7 +196,7 @@ defmodule Styler.Style.ModuleDirectives do
     shortdocs = directives[:"@shortdoc"] || []
     moduledocs = directives[:"@moduledoc"] || List.wrap(moduledoc)
     behaviours = expand_and_sort(directives[:"@behaviour"] || [])
-    uses = (directives[:use] || []) |> Enum.flat_map(&expand_directive/1) |> reset_newlines()
+    uses = (directives[:use] || []) |> Enum.flat_map(&expand_directive/1) |> Style.reset_newlines()
     imports = expand_and_sort(directives[:import] || [])
     aliases = expand_and_sort(directives[:alias] || [])
     requires = expand_and_sort(directives[:require] || [])
@@ -214,7 +214,7 @@ defmodule Styler.Style.ModuleDirectives do
         requires
       ]
       |> Enum.concat()
-      |> fix_line_numbers(List.first(nondirectives))
+      |> Style.fix_line_numbers(List.first(nondirectives))
 
     # the # of aliases can be decreased during sorting - if there were any, we need to be sure to write the deletion
     if Enum.empty?(directives) do
@@ -323,61 +323,6 @@ defmodule Styler.Style.ModuleDirectives do
     |> Zipper.node()
   end
 
-  # This is the step that ensures that comments don't get wrecked as part of us moving AST nodes willy-nilly.
-  #
-  # For example, given document
-  #
-  # 1: defmodule ...
-  # 2: alias B
-  # 3: # hi
-  # 4: # this is foo
-  # 5: def foo ...
-  # 6: alias A
-  #
-  # Moving the ast node for alias A would put line 6 before line 2 in the AST.
-  # Elixir's document algebra would then encounter "line 6" and immediately dump all comments with line < 6,
-  # meaning after running through the formatter we'd end up with
-  #
-  # 1: defmodule
-  # 2: # hi
-  # 3: # this is foo
-  # 4: alias A
-  # 5: alias B
-  # 6:
-  # 7: def foo ...
-  #
-  # This fixes that error by ensuring the following property:
-  # A given node of AST cannot have a line number greater than the next AST node.
-  # Et voila! Comments behave much better.
-  defp fix_line_numbers(directives, acc \\ [], first_non_directive)
-
-  defp fix_line_numbers([this, next | rest], acc, first_non_directive) do
-    this = cap_line(this, next)
-    fix_line_numbers([next | rest], [this | acc], first_non_directive)
-  end
-
-  defp fix_line_numbers([last], acc, first_non_directive) do
-    last = if first_non_directive, do: cap_line(last, first_non_directive), else: last
-    Enum.reverse([last | acc])
-  end
-
-  defp fix_line_numbers([], [], _), do: []
-
-  defp cap_line({_, this_meta, _} = this, {_, next_meta, _}) do
-    this_line = this_meta[:line]
-    next_line = next_meta[:line]
-
-    if this_line > next_line do
-      # Subtracting 2 helps the behaviour with one-liner comments preceding the next node. It's a bit of a hack.
-      # TODO: look into the comments list and
-      # 1. move comment blocks preceding `this` up with it
-      # 2. find the earliest comment before `next` and set `new_line` to that value - 1
-      Style.set_line(this, next_line - 2, delete_newlines: false)
-    else
-      this
-    end
-  end
-
   defp expand_and_sort(directives) do
     # sorting is done with `downcase` to match Credo
     directives
@@ -386,7 +331,7 @@ defmodule Styler.Style.ModuleDirectives do
     |> Enum.uniq_by(&elem(&1, 1))
     |> List.keysort(1)
     |> Enum.map(&elem(&1, 0))
-    |> reset_newlines()
+    |> Style.reset_newlines()
   end
 
   # Deletes root level aliases ala (`alias Foo` -> ``)
@@ -410,15 +355,4 @@ defmodule Styler.Style.ModuleDirectives do
   end
 
   defp expand_directive(other), do: [other]
-
-  defp reset_newlines([]), do: []
-  defp reset_newlines(directives), do: reset_newlines(directives, [])
-
-  defp reset_newlines([directive], acc), do: Enum.reverse([set_newlines(directive, 2) | acc])
-  defp reset_newlines([directive | rest], acc), do: reset_newlines(rest, [set_newlines(directive, 1) | acc])
-
-  defp set_newlines({directive, meta, children}, newline) do
-    updated_meta = Keyword.update(meta, :end_of_expression, [newlines: newline], &Keyword.put(&1, :newlines, newline))
-    {directive, updated_meta, children}
-  end
 end
diff --git a/lib/styler.ex b/lib/styler.ex
index 2015977..ede22bb 100644
--- a/lib/styler.ex
+++ b/lib/styler.ex
@@ -24,19 +24,22 @@ defmodule Styler do
     Styler.Style.SingleNode,
     Styler.Style.Defs,
     Styler.Style.Blocks,
-    Styler.Style.Deprecations
+    Styler.Style.Deprecations,
+    Styler.Style.Configs
   ]
 
   @doc false
   def style({ast, comments}, file, opts) do
     on_error = opts[:on_error] || :log
     zipper = Zipper.zip(ast)
-    context = %{comments: comments, file: file}
 
-    {{ast, _}, %{comments: comments}} =
-      Enum.reduce(@styles, {zipper, context}, fn style, {zipper, context} ->
+    {{ast, _}, comments} =
+      Enum.reduce(@styles, {zipper, comments}, fn style, {zipper, comments} ->
+        context = %{comments: comments, file: file}
+
         try do
-          Zipper.traverse_while(zipper, context, &style.run/2)
+          {zipper, %{comments: comments}} = Zipper.traverse_while(zipper, context, &style.run/2)
+          {zipper, comments}
         rescue
           exception ->
             exception = StyleError.exception(exception: exception, style: style, file: file)


Test patch:
diff --git a/test/style/configs_test.exs b/test/style/configs_test.exs
new file mode 100644
index 0000000..3a36dd6
--- /dev/null
+++ b/test/style/configs_test.exs
@@ -0,0 +1,135 @@
+# Copyright 2024 Adobe. All rights reserved.
+# This file is licensed to you under the Apache License, Version 2.0 (the "License");
+# you may not use this file except in compliance with the License. You may obtain a copy
+# of the License at http://www.apache.org/licenses/LICENSE-2.0
+
+# Unless required by applicable law or agreed to in writing, software distributed under
+# the License is distributed on an "AS IS" BASIS, WITHOUT WARRANTIES OR REPRESENTATIONS
+# OF ANY KIND, either express or implied. See the License for the specific language
+# governing permissions and limitations under the License.
+
+defmodule Styler.Style.ConfigsTest do
+  @moduledoc false
+  use Styler.StyleCase, async: true, filename: "config/config.exs"
+
+  alias Styler.Style.Configs
+
+  test "only runs on exs files in config folders" do
+    {ast, _} = Styler.string_to_quoted_with_comments("import Config\n\nconfig :bar, boop: :baz")
+    zipper = Styler.Zipper.zip(ast)
+
+    for file <- ~w(dev.exs my_app.exs config.exs) do
+      # :config? is private api, so don't be surprised if this has to change at some point
+      assert {:cont, _, %{config?: true}} = Configs.run(zipper, %{file: "apps/foo/config/#{file}"})
+      assert {:cont, _, %{config?: true}} = Configs.run(zipper, %{file: "config/#{file}"})
+      assert {:halt, _, _} = Configs.run(zipper, %{file: file})
+    end
+  end
+
+  test "orders configs stanzas" do
+    # doesn't order when we haven't seen `import Config`, so this is something else that we don't understand
+    assert_style """
+    config :z, :x
+    config :a, :b
+    """
+
+    # 1. orders `config/2,3` relative to each other
+    # 2. lifts assignments above config blocks
+    # 3. non assignment/config separate "config" blocks
+
+    assert_style(
+      """
+      import Config
+      dog_sound = :woof
+      # z is best when configged w/ dog sounds
+      # dog sounds ftw
+      config :z, :x, dog_sound
+
+      # this is my big c
+      # comment i'd like to leave c
+      # about c
+      c = :c
+      config :a, :b, c
+      config :a, :c, :d
+      config :a,
+        a_longer_name: :a_longer_value,
+        multiple_things: :that_could_all_fit_on_one_line_though
+
+      # this is my big my_app
+      # comment i'd like to leave my_app
+      # about my_app
+      my_app =
+        :"dont_write_configs_like_this_yall_:("
+
+      # this is my big your_app
+      # comment i'd like to leave your_app
+      # about your_app
+      your_app = :not_again!
+      config your_app, :dont_use_varrrrrrrrs
+      config my_app, :nooooooooo
+      import_config "my_config"
+
+      cat_sound = :meow
+      config :z, a: :meow
+      a_sad_overwrite_that_will_be_hard_to_notice = :x
+      config :a, :b, a_sad_overwrite_that_will_be_hard_to_notice
+      """,
+      """
+      import Config
+
+      dog_sound = :woof
+      # z is best when configged w/ dog sounds
+      # dog sounds ftw
+
+      # this is my big c
+      # comment i'd like to leave c
+      # about c
+      c = :c
+
+      # this is my big my_app
+      # comment i'd like to leave my_app
+      # about my_app
+      my_app =
+        :"dont_write_configs_like_this_yall_:("
+
+      # this is my big your_app
+      # comment i'd like to leave your_app
+      # about your_app
+      your_app = :not_again!
+
+      config :a, :b, c
+      config :a, :c, :d
+
+      config :a,
+        a_longer_name: :a_longer_value,
+        multiple_things: :that_could_all_fit_on_one_line_though
+
+      config :z, :x, dog_sound
+
+      config my_app, :nooooooooo
+
+      config your_app, :dont_use_varrrrrrrrs
+
+      import_config "my_config"
+
+      cat_sound = :meow
+      a_sad_overwrite_that_will_be_hard_to_notice = :x
+
+      config :a, :b, a_sad_overwrite_that_will_be_hard_to_notice
+
+      config :z, a: :meow
+      """
+    )
+  end
+
+  test "ignores things that look like config/1" do
+    assert_style """
+    import Config
+
+    config :a, :b
+
+    config(a)
+    config :c, :d
+    """
+  end
+end
diff --git a/test/support/style_case.ex b/test/support/style_case.ex
index 1d0225e..80627c5 100644
--- a/test/support/style_case.ex
+++ b/test/support/style_case.ex
@@ -14,9 +14,12 @@ defmodule Styler.StyleCase do
   """
   use ExUnit.CaseTemplate
 
-  using do
+  using options do
     quote do
-      import unquote(__MODULE__), only: [assert_style: 1, assert_style: 2, style: 1, format_diff: 2, format_diff: 3]
+      import unquote(__MODULE__),
+        only: [assert_style: 1, assert_style: 2, style: 1, style: 2, format_diff: 2, format_diff: 3]
+
+      @filename unquote(options)[:filename] || "testfile"
     end
   end
 
@@ -27,7 +30,7 @@ defmodule Styler.StyleCase do
       alias Styler.Zipper
 
       expected = String.trim(expected)
-      {styled_ast, styled, styled_comments} = style(before)
+      {styled_ast, styled, styled_comments} = style(before, @filename)
 
       if styled != expected and ExUnit.configuration()[:trace] do
         IO.puts("\n======Given=============\n")
@@ -102,7 +105,7 @@ defmodule Styler.StyleCase do
       end)
 
       # Idempotency
-      {_, restyled, _} = style(styled)
+      {_, restyled, _} = style(styled, @filename)
 
       if restyled == styled do
         assert true
@@ -114,9 +117,9 @@ defmodule Styler.StyleCase do
     end
   end
 
-  def style(code) do
+  def style(code, filename \\ "testfile") do
     {ast, comments} = Styler.string_to_quoted_with_comments(code)
-    {styled_ast, comments} = Styler.style({ast, comments}, "testfile", on_error: :raise)
+    {styled_ast, comments} = Styler.style({ast, comments}, filename, on_error: :raise)
 
     try do
       styled_code = styled_ast |> Styler.quoted_to_string(comments) |> String.trim_trailing("\n")