From 7dbe07af464ad52cf5c75aae2ef3d56c91e17c54 Mon Sep 17 00:00:00 2001 From: Christian Koch Date: Fri, 17 Sep 2021 20:52:18 -0500 Subject: [PATCH] Add dry run option and tests --- lib/ex_factor/extractor.ex | 40 +++++++------- lib/ex_factor/remover.ex | 26 +++++++-- test/ex_factor/extractor_test.exs | 92 +++++++++++++++++-------------- test/ex_factor/parser_test.exs | 1 + test/ex_factor/remover_test.exs | 86 ++++++++++++++++++++++++++++- 5 files changed, 179 insertions(+), 66 deletions(-) diff --git a/lib/ex_factor/extractor.ex b/lib/ex_factor/extractor.ex index 888ebc9..2b3af05 100644 --- a/lib/ex_factor/extractor.ex +++ b/lib/ex_factor/extractor.ex @@ -4,7 +4,6 @@ defmodule ExFactor.Extractor do """ alias ExFactor.Neighbors alias ExFactor.Parser - alias ExFactor.Remover @doc """ Given a keyword list of opts, find the function in the specified source. @@ -29,51 +28,52 @@ defmodule ExFactor.Extractor do arity = Keyword.fetch!(opts, :arity) target_path = Keyword.get(opts, :target_path, path(target_module)) source_path = Keyword.get(opts, :source_path, path(source_module)) + dry_run = Keyword.get(opts, :dry_run, false) {_ast, block_contents} = Parser.block_contents(source_path) - to_extract = Neighbors.walk(block_contents, source_function, arity) + to_extract = block_contents + |> Neighbors.walk(source_function, arity) + |> Enum.map(&(Macro.to_string(&1))) + |> IO.inspect(label: "to string") + + string_fns = Enum.join(to_extract, "\n") case File.exists?(target_path) do true -> {ast, list} = Parser.read_file(target_path) {:defmodule, [do: [line: _begin_line], end: [line: end_line], line: _], _} = ast - string_fns = Macro.to_string(to_extract) + # string_fns = Macro.to_string(to_extract) + # string_fns = Enum.join(to_extract, "\n") list |> List.insert_at(end_line - 1, refactor_message()) |> List.insert_at(end_line, string_fns) |> Enum.join("\n") - |> then(fn contents -> File.write(target_path, contents, [:write]) end) + |> then(fn contents -> write_file(target_path, contents, dry_run) end) _ -> - content = + contents = quote generated: true do defmodule unquote(target_module) do @moduledoc false - unquote(to_extract) + unquote(Macro.unescape_string(string_fns)) end end |> Macro.to_string() - File.write(target_path, content) + write_file(target_path, contents, dry_run) end end - @doc """ - Remove the indicated function and its spec from it's original file. - """ - def remove(opts) do - source_module = Keyword.fetch!(opts, :source_module) - source_function = Keyword.fetch!(opts, :source_function) - arity = Keyword.fetch!(opts, :arity) - source_path = Keyword.get(opts, :source_path, path(source_module)) - |> IO.inspect(label: "REMOVE source_path") - - Remover.remove(source_path, source_function, arity) - end - defp path(module) do Path.join(["lib", Macro.underscore(module) <> ".ex"]) end defp refactor_message, do: "#refactored function moved with ExFactor" + + defp write_file(_target_path, contents, true) do + contents + end + defp write_file(target_path, contents, _) do + File.write(target_path, contents, [:write]) + end end diff --git a/lib/ex_factor/remover.ex b/lib/ex_factor/remover.ex index d51c765..ea47595 100644 --- a/lib/ex_factor/remover.ex +++ b/lib/ex_factor/remover.ex @@ -5,9 +5,19 @@ defmodule ExFactor.Remover do alias ExFactor.Parser - def remove(source_path, fn_name, arity) do + @doc """ + Remove the indicated function and its spec from it's original file. + """ + def remove(opts) do + source_function = Keyword.fetch!(opts, :source_function) + arity = Keyword.fetch!(opts, :arity) + source_path = Keyword.fetch!(opts, :source_path) + dry_run = Keyword.get(opts, :dry_run, false) + # |> IO.inspect(label: "REMOVE source_path") + + {_ast, block_contents} = Parser.all_functions(source_path) - fns_to_remove = Enum.filter(block_contents, &(&1.name == fn_name)) + fns_to_remove = Enum.filter(block_contents, &(&1.name == source_function)) {_ast, line_list} = Parser.read_file(source_path) Enum.reduce(fns_to_remove, line_list, fn function, acc -> @@ -20,10 +30,10 @@ defmodule ExFactor.Remover do |> Enum.reduce(acc, fn idx, acc -> List.delete_at(acc, idx - 1) end) - |> List.insert_at(function.start_line, comment(fn_name, arity, function.defn)) + |> List.insert_at(function.start_line, comment(source_function, arity, function.defn)) end) |> Enum.join("\n") - |> then(fn str -> File.write(source_path, str, [:write]) end) + |> then(fn str -> write_file(source_path, str, dry_run) end) end defp comment(name, arity, "@spec") do @@ -42,4 +52,12 @@ defmodule ExFactor.Remover do # """ end + + defp write_file(_path, contents, true) do + contents + end + + defp write_file(path, contents, _) do + File.write(path, contents, [:write]) + end end diff --git a/test/ex_factor/extractor_test.exs b/test/ex_factor/extractor_test.exs index b91c3d8..97ea25d 100644 --- a/test/ex_factor/extractor_test.exs +++ b/test/ex_factor/extractor_test.exs @@ -2,17 +2,23 @@ defmodule ExFactor.ExtractorTest do use ExUnit.Case alias ExFactor.Extractor + setup_all do + File.mkdir_p("test/tmp") + + on_exit(fn -> + File.rm_rf("test/tmp") + end) + end + describe "emplace/1" do test "requires some options" do opts = [ - # target_module: ExFactor.NewMod, source_module: ExFactorSampleModule, source_function: :pub1, arity: 1 ] assert_raise KeyError, "key :target_module not found in: #{inspect(opts)}", fn -> Extractor.emplace(opts) end - # assert message == "" end test "write a new file with the function" do @@ -28,16 +34,16 @@ defmodule ExFactor.ExtractorTest do end """ - File.write("test/support/source_module.ex", content) + File.write("test/tmp/source_module.ex", content) - target_path = "test/support/target_module.ex" + target_path = "test/tmp/target_module.ex" File.rm(target_path) opts = [ target_path: target_path, target_module: ExFactor.NewMod, source_module: ExFactorSampleModule, - source_path: "test/support/source_module.ex", + source_path: "test/tmp/source_module.ex", source_function: :pub1, arity: 1 ] @@ -50,10 +56,45 @@ defmodule ExFactor.ExtractorTest do # includes additional attrs assert file =~ "@spec(pub1(term()) :: term())" assert file =~ "@somedoc(\"This is somedoc\")" + # assert the added elements get flattened correctly + refute file =~ "[@somedoc(\"This is somedoc\"), " # comments don't get moved refute file =~ "# a comment and no aliases" - File.rm("test/support/source_module.ex") - File.rm("test/support/target_module.ex") + File.rm("test/tmp/source_module.ex") + File.rm("test/tmp/target_module.ex") + end + + test " with dry_run option, don't write the file." do + content = """ + defmodule ExFactorSampleModule do + @somedoc "This is somedoc" + # a comment and no aliases + _docp = "here's an arbitrary module underscore" + @spec pub1(term()) :: term() + def pub1(arg1) do + :ok + end + end + """ + + File.write("test/tmp/source_module.ex", content) + + target_path = "test/tmp/target_module.ex" + File.rm(target_path) + + opts = [ + target_path: target_path, + target_module: ExFactor.NewMod, + source_module: ExFactorSampleModule, + source_path: "test/tmp/source_module.ex", + source_function: :pub1, + arity: 1, + dry_run: true + ] + + output = Extractor.emplace(opts) + assert {:error, :enoent} = File.read(target_path) + assert output =~ "defmodule(ExFactor.NewMod) do" end test "write a new file with the function, infer some defaults" do @@ -101,7 +142,7 @@ defmodule ExFactor.ExtractorTest do end """ - File.write("lib/ex_factor/source_module.ex", content) + File.write("test/tmp/source_module.ex", content) content = """ defmodule ExFactor.TargetModule do @@ -203,6 +244,7 @@ defmodule ExFactor.ExtractorTest do content = """ defmodule ExFactor.TargetModule do + @moduledoc false @somedoc "This is somedoc TargetModule" @doc "some docs" def pub_exists(arg_exists) do @@ -226,6 +268,7 @@ defmodule ExFactor.ExtractorTest do Extractor.emplace(opts) file = File.read!("lib/ex_factor/target_module.ex") + |> IO.inspect(label: "") assert file =~ "def(refactor1(arg1)) do" assert file =~ "def(refactor1([])) do" assert file =~ " @doc \"some docs\"" @@ -234,37 +277,4 @@ defmodule ExFactor.ExtractorTest do File.rm("lib/ex_factor/target_module.ex") end end - - describe "remove/1" do - test "remove the given function from the source module" do - content = """ - defmodule ExFactorSampleModule do - @somedoc "This is somedoc" - # a comment and no aliases - _docp = "here's an arbitrary module underscore" - def pub1(arg1) do - :ok - end - end - """ - - File.write("test/support/source_module.ex", content) - source_path = "test/support/source_module.ex" - - opts = [ - source_module: ExFactorSampleModule, - source_path: "test/support/source_module.ex", - source_function: :pub1, - arity: 1 - ] - - Extractor.remove(opts) - - file = File.read!(source_path) - assert file =~ "defmodule ExFactorSampleModule do" - assert file =~ "_docp = \"here's an arbitrary module underscore" - refute file =~ "def pub1(arg1) do" - File.rm("test/support/source_module.ex") - end - end end diff --git a/test/ex_factor/parser_test.exs b/test/ex_factor/parser_test.exs index 1e47e77..3e4f8b9 100644 --- a/test/ex_factor/parser_test.exs +++ b/test/ex_factor/parser_test.exs @@ -439,6 +439,7 @@ defmodule ExFactor.ParserTest do test "it reads the file into an AST and list of each line" do content = """ defmodule ExFactorSampleModule do + alias ExFactorOtherModule @somedoc "This is somedoc" # comments get dropped _docp = "arbitrary module-level elem" diff --git a/test/ex_factor/remover_test.exs b/test/ex_factor/remover_test.exs index 6b9dca7..dc7c9a1 100644 --- a/test/ex_factor/remover_test.exs +++ b/test/ex_factor/remover_test.exs @@ -11,6 +11,37 @@ defmodule ExFactor.RemoverTest do end describe "remove/1" do + test "remove the given function from the source module" do + content = """ + defmodule ExFactorSampleModule do + @somedoc "This is somedoc" + # a comment and no aliases + _docp = "here's an arbitrary module underscore" + def pub1(arg1) do + :ok + end + end + """ + + File.write("test/tmp/source_module.ex", content) + source_path = "test/tmp/source_module.ex" + + opts = [ + source_module: ExFactorSampleModule, + source_path: "test/tmp/source_module.ex", + source_function: :pub1, + arity: 1 + ] + + Remover.remove(opts) + + file = File.read!(source_path) + assert file =~ "defmodule ExFactorSampleModule do" + assert file =~ "_docp = \"here's an arbitrary module underscore" + refute file =~ "def pub1(arg1) do" + File.rm("test/support/source_module.ex") + end + test "it rewrites the source file and removes code blocks" do module = """ defmodule ExFactorSampleModule do @@ -38,7 +69,13 @@ defmodule ExFactor.RemoverTest do """ File.write("test/tmp/source_module.ex", module) - Remover.remove("test/tmp/source_module.ex", :pub1, 1) + opts = [ + source_module: ExFactorSampleModule, + source_path: "test/tmp/source_module.ex", + source_function: :pub1, + arity: 1 + ] + Remover.remove(opts) updated_file = File.read!("test/tmp/source_module.ex") refute updated_file =~ "def pub1(arg1) do" @@ -47,5 +84,52 @@ defmodule ExFactor.RemoverTest do # it removes specs too refute updated_file =~ "@spec pub1(term()) :: term()" end + + test "takes a dry_run option to only report intended changes" do + module = """ + defmodule ExFactorSampleModule do + @somedoc "This is somedoc" + # comments get dropped + @doc " + multiline + documentation for pub1 + " + @spec pub1(term()) :: term() + def pub1(arg1) do + :ok + end + _docp = "arbitrary module-level elem" + defp priv1(arg1) do + :ok + end + + def pub2(arg1) + do + :ok + end + + end + """ + + File.write("test/tmp/source_module.ex", module) + opts = [ + dry_run: true, + source_module: ExFactorSampleModule, + source_path: "test/tmp/source_module.ex", + source_function: :pub1, + arity: 1 + ] + changes = Remover.remove(opts) + + unchanged_file = File.read!("test/tmp/source_module.ex") + assert unchanged_file =~ "def pub1(arg1) do" + refute unchanged_file =~ "Function: pub1/1 removed by ExFactor" + assert unchanged_file =~ "@spec pub1(term()) :: term()" + + assert changes =~ "Function: pub1/1 removed by ExFactor" + refute changes =~ "@spec pub1(term()) :: term()" + refute changes =~ "def pub1(arg1) do" + end + end end