From a8c0222e9712a32fb3c1aa6e60d9fac66b29253a Mon Sep 17 00:00:00 2001 From: Zach Daniel Date: Tue, 10 Sep 2024 11:15:28 -0400 Subject: [PATCH] fix: properly avoid adding duplicate children to application tree --- lib/igniter.ex | 21 +++++++++++++++++- lib/igniter/code/common.ex | 8 +++---- lib/igniter/project/application.ex | 16 ++++++-------- lib/igniter/test.ex | 26 +++++++++++++++++++++++ test/igniter/project/application_test.exs | 24 +++++++++++++++++++++ 5 files changed, 80 insertions(+), 15 deletions(-) diff --git a/lib/igniter.ex b/lib/igniter.ex index b18bf41..7a09629 100644 --- a/lib/igniter.ex +++ b/lib/igniter.ex @@ -1344,7 +1344,26 @@ defmodule Igniter do end end - defp changed?(source) do + @doc "Returns true if any of the files specified in `paths` have changed." + @spec changed?(t(), String.t() | list(String.t())) :: boolean() + def changed?(%Igniter{} = igniter, paths) do + paths = List.wrap(paths) + + igniter.rewrite + |> Rewrite.sources() + |> Enum.filter(&(&1.path in paths)) + |> Enum.any?(&changed?/1) + end + + @doc "Returns true if the igniter or source provided has changed" + @spec changed?(t() | Rewrite.Source.t()) :: boolean() + def changed?(%Igniter{} = igniter) do + igniter.rewrite + |> Rewrite.sources() + |> Enum.any?(&changed?/1) + end + + def changed?(%Rewrite.Source{} = source) do diff = Rewrite.Source.diff(source) |> IO.iodata_to_binary() String.trim(diff) != "" diff --git a/lib/igniter/code/common.ex b/lib/igniter/code/common.ex index af5772e..00451ee 100644 --- a/lib/igniter/code/common.ex +++ b/lib/igniter/code/common.ex @@ -747,7 +747,7 @@ defmodule Igniter.Code.Common do def nodes_equal?(v, v), do: true def nodes_equal?(l, r) do - equal_vals?(l, r) || equal_modules?(l, r) + equal_vals?(l, r) end defp equal_vals?({:__block__, _, [value]}, value) do @@ -766,7 +766,7 @@ defmodule Igniter.Code.Common do nodes_equal?(left, right) _ -> - false + equal_modules?(left, right) end extendable_block?(right) -> @@ -775,7 +775,7 @@ defmodule Igniter.Code.Common do nodes_equal?(left, right) _ -> - false + equal_modules?(left, right) end is_list(left) and is_list(right) -> @@ -783,7 +783,7 @@ defmodule Igniter.Code.Common do Enum.all?(Enum.zip(left, right), fn {l, r} -> nodes_equal?(l, r) end) true -> - false + equal_modules?(left, right) end end diff --git a/lib/igniter/project/application.ex b/lib/igniter/project/application.ex index ff3c8c9..3b61c4e 100644 --- a/lib/igniter/project/application.ex +++ b/lib/igniter/project/application.ex @@ -178,20 +178,16 @@ defmodule Igniter.Project.Application do {:ok, zipper} <- Igniter.Code.Function.move_to_nth_argument(zipper, 1) do if Igniter.Code.List.find_list_item_index(zipper, fn item -> if Igniter.Code.Tuple.tuple?(item) do - with {:ok, zipper} <- Igniter.Code.Tuple.tuple_elem(zipper, 0), - zipper <- Igniter.Code.Common.expand_alias(zipper), - module when is_atom(module) <- alias_to_mod(zipper.node) do - module == to_supervise_module + with {:ok, item} <- Igniter.Code.Tuple.tuple_elem(item, 0), + item <- Igniter.Code.Common.expand_alias(item) do + Igniter.Code.Common.nodes_equal?(item, to_supervise_module) else _ -> false end else - with zipper <- Igniter.Code.Common.expand_alias(zipper), - module when is_atom(module) <- zipper.node do - module == to_supervise_module - else - _ -> false - end + item + |> Igniter.Code.Common.expand_alias() + |> Igniter.Code.Common.nodes_equal?(to_supervise_module) end end) do {:ok, zipper} diff --git a/lib/igniter/test.ex b/lib/igniter/test.ex index efc320c..2577b51 100644 --- a/lib/igniter/test.ex +++ b/lib/igniter/test.ex @@ -143,6 +143,32 @@ defmodule Igniter.Test do end end + defmacro assert_unchanged(igniter, path_or_paths) do + quote bind_quoted: [igniter: igniter, path_or_paths: path_or_paths] do + for path <- List.wrap(path_or_paths) do + refute Igniter.changed?(igniter, path), """ + Expected #{inspect(path)} to be unchanged, but it was changed. + + Diff: + + #{igniter.rewrite.sources |> Map.take([path]) |> Igniter.diff()} + """ + end + end + end + + defmacro assert_unchanged(igniter) do + quote bind_quoted: [igniter: igniter] do + refute Igniter.changed?(igniter), """ + Expected there to be no changes, but there were changes. + + Diff: + + #{Rewrite.sources(igniter.rewrite) |> Igniter.diff()} + """ + end + end + @doc false def sanitize_diff(diff, actual \\ nil) do diff diff --git a/test/igniter/project/application_test.exs b/test/igniter/project/application_test.exs index a7e51d9..af55b0a 100644 --- a/test/igniter/project/application_test.exs +++ b/test/igniter/project/application_test.exs @@ -28,6 +28,30 @@ defmodule Igniter.Project.ApplicationTest do """) end + test "doesnt add a module if its already supervised" do + test_project() + |> Igniter.Project.Application.add_new_child(Foo) + |> apply_igniter!() + |> Igniter.Project.Application.add_new_child(Foo) + |> assert_unchanged() + end + + test "doesnt add a module if its already supervised as a tuple" do + test_project() + |> Igniter.Project.Application.add_new_child({Foo, a: 1}) + |> apply_igniter!() + |> Igniter.Project.Application.add_new_child(Foo) + |> assert_unchanged() + end + + test "doesnt add a module if its already supervised as an atom and we're adding a tuple" do + test_project() + |> Igniter.Project.Application.add_new_child(Foo) + |> apply_igniter!() + |> Igniter.Project.Application.add_new_child({Foo, a: 1}) + |> assert_unchanged() + end + test "supports taking options as the second argument" do test_project() |> Igniter.Project.Application.add_new_child({Foo, a: :b})