Refactor chunk_extractor api

simplify the logic, get rid of direction parameter
now it only tries to retrieve the chunk from the top to the down
got rid of direction param and custom chunk.sh script
use shell sed command

Author: RudolfMan

lib/diff/hex/chunk_extractor.ex

@@ -1,20 +1,28 @@
 defmodule Diff.Hex.ChunkExtractor do
-  @enforce_keys [:file_path, :from_line, :lines_to_read, :direction]
-  defstruct Enum.map(@enforce_keys, &{&1, nil}) ++
-              [
-                errors: [],
-                raw: nil,
-                parsed: nil
-              ]
+  @enforce_keys [:file_path, :from_line, :to_line, :right_line]
+  defstruct Enum.map(@enforce_keys, &{&1, nil}) ++ [errors: [], raw: nil, parsed: nil]
 
   def run(params) do
-    __MODULE__
-    |> struct!(params)
-    |> parse_integers([:from_line, :lines_to_read])
-    |> validate_direction()
+    params
+    |> cast()
+    |> parse_integers([:right_line, :from_line, :to_line])
+    |> normalize([:from_line, :right_line])
+    |> validate_to_line()
     |> system_read_raw_chunk()
     |> parse_chunk()
-    |> remove_trailing_newline()
+    |> case do
+      %{errors: []} = chunk -> {:ok, chunk}
+      %{errors: _} = chunk -> {:error, chunk}
+    end
+  end
+
+  defp cast(params) do
+    struct_keys =
+      struct(__MODULE__)
+      |> Map.from_struct()
+      |> Enum.map(fn {key, _val} -> key end)
+
+    struct!(__MODULE__, Map.take(params, struct_keys))
   end
 
   defp parse_integers(chunk, fields) do
@@ -25,7 +33,7 @@ defmodule Diff.Hex.ChunkExtractor do
     value = chunk |> Map.get(field) |> parse_value()
 
     case value do
-      :error -> %{chunk | errors: {:parse_integer, "#{field} must be a number"}}
+      :error -> %{chunk | errors: [{:parse_integer, "#{field} must be a number"} | chunk.errors]}
       integer -> Map.put(chunk, field, integer)
     end
   end
@@ -36,26 +44,39 @@ defmodule Diff.Hex.ChunkExtractor do
     with {int, _} <- Integer.parse(number), do: int
   end
 
-  defp validate_direction(%{direction: direction} = chunk) when direction in ["up", "down"] do
+  defp normalize(%{errors: [_ | _]} = chunk, _), do: chunk
+
+  defp normalize(chunk, fields) do
+    Enum.reduce(fields, chunk, &normalize_field/2)
+  end
+
+  defp normalize_field(field, chunk) do
+    case Map.fetch!(chunk, field) do
+      value when value > 0 -> chunk
+      _ -> Map.put(chunk, field, 1)
+    end
+  end
+
+  defp validate_to_line(%{errors: [_ | _]} = chunk), do: chunk
+
+  defp validate_to_line(%{from_line: from_line, to_line: to_line} = chunk)
+       when from_line < to_line do
     chunk
   end
 
-  defp validate_direction(chunk) do
-    error = {:direction, "direction must be either \"up\" or \"down\""}
+  defp validate_to_line(chunk) do
+    error = {:param, "from_line parameter must be less than to_line"}
     %{chunk | errors: [error | chunk.errors]}
   end
 
   defp system_read_raw_chunk(%{errors: [_ | _]} = chunk), do: chunk
 
   defp system_read_raw_chunk(chunk) do
-    chunk_sh = Application.app_dir(:diff, ["priv", "chunk.sh"])
-
     path = chunk.file_path
     from_line = to_string(chunk.from_line)
-    lines_to_read = to_string(chunk.lines_to_read)
-    direction = chunk.direction
+    to_line = to_string(chunk.to_line)
 
-    case System.cmd(chunk_sh, [path, from_line, lines_to_read, direction], stderr_to_stdout: true) do
+    case System.cmd("sed", ["-n", "#{from_line},#{to_line}p", path], stderr_to_stdout: true) do
       {raw_chunk, 0} ->
         %{chunk | raw: raw_chunk}
 
@@ -71,42 +92,21 @@ defmodule Diff.Hex.ChunkExtractor do
     parsed =
       chunk.raw
       |> String.split("\n")
-      |> Enum.map(fn line -> %{line_text: line} end)
-
-    set_line_numbers(%{chunk | parsed: parsed})
+      |> remove_trailing_newline()
+      |> Enum.with_index(chunk.from_line)
+      |> Enum.with_index(chunk.right_line)
+      # prepend line with whitespace to compensate the space introduced by leading + and - in diffs
+      |> Enum.map(fn {{line, from}, to} ->
+        %{text: " " <> line, from_line_number: from, to_line_number: to}
+      end)
+
+    %{chunk | parsed: parsed}
   end
 
-  defp set_line_numbers(%{direction: "down"} = chunk) do
-    %{chunk | parsed: parsed_with_line_numbers(chunk.parsed, chunk.from_line)}
-  end
-
-  defp set_line_numbers(%{direction: "up"} = chunk) do
-    offset = chunk.from_line - length(chunk.parsed) + 1
-
-    %{chunk | parsed: parsed_with_line_numbers(chunk.parsed, offset)}
-  end
-
-  defp parsed_with_line_numbers(parsed_chunk, starting_number) when is_binary(starting_number) do
-    parsed_with_line_numbers(parsed_chunk, starting_number)
-  end
-
-  defp parsed_with_line_numbers(parsed_chunk, starting_number) do
-    parsed_chunk
-    |> Enum.with_index(starting_number)
-    |> Enum.map(fn {line, line_number} -> Map.put_new(line, :line_number, line_number) end)
-  end
-
-  defp remove_trailing_newline(%{errors: [_ | _]} = chunk), do: {:error, chunk}
-
-  defp remove_trailing_newline(chunk) do
-    [trailing_line | reversed_tail] = Enum.reverse(chunk.parsed)
-
-    chunk =
-      case trailing_line do
-        %{line_text: ""} -> %{chunk | parsed: Enum.reverse(reversed_tail)}
-        %{line_text: _text} -> chunk
-      end
-
-    {:ok, chunk}
+  defp remove_trailing_newline(lines) do
+    lines
+    |> Enum.reverse()
+    |> tl()
+    |> Enum.reverse()
   end
 end

lib/diff/hex/hex.ex

@@ -101,19 +101,14 @@ defmodule Diff.Hex do
     end
   end
 
-  def get_chunk(package, version, file_name, from_line, direction) do
-    path = tmp_path("package-#{package}-#{version}-")
+  def get_chunk(params) do
+    path = tmp_path("chunk")
 
-    chunk_extractor_params = %{
-      file_path: Path.join(path, file_name),
-      from_line: from_line,
-      direction: direction,
-      lines_to_read: 20
-    }
+    chunk_extractor_params = Map.put(params, :file_path, Path.join(path, params.file_name))
 
     try do
-      with {:ok, tarball} <- get_tarball(package, version),
-           :ok <- unpack_tarball(tarball, [file_name], path),
+      with {:ok, tarball} <- get_tarball(params.package, params.version),
+           :ok <- unpack_tarball(tarball, [params.file_name], path),
            {:ok, %{parsed: parsed_chunk}} <- Diff.Hex.ChunkExtractor.run(chunk_extractor_params) do
         {:ok, parsed_chunk}
       else

priv/chunk.sh

@@ -17,72 +17,57 @@ defmodule Diff.Hex.ChunkExtractorTest do
     on_exit(fn -> File.rm!(path) end)
 
     # some deafult params
-    %{params: %{file_path: path, lines_to_read: 2, from_line: 1, direction: "down"}}
-  end
-
-  describe "validates direction" do
-    test "down", %{params: params} do
-      {:ok, %{errors: errors}} = ChunkExtractor.run(%{params | direction: "down"})
-      assert [] = errors
-    end
-
-    test "up", %{params: params} do
-      {:ok, %{errors: errors}} = ChunkExtractor.run(%{params | direction: "up"})
-      assert [] = errors
-    end
-
-    test "error when direction is neither up nor down", %{params: params} do
-      {:error, %{errors: errors}} = ChunkExtractor.run(%{params | direction: "left"})
-      assert "direction must be either \"up\" or \"down\"" = Keyword.get(errors, :direction)
-    end
+    %{
+      params: %{file_path: path, right_line: 1, from_line: 1, to_line: 2}
+    }
   end
 
   describe "reads raw chunk from the file_path" do
     test "reads first 2 lines down", %{params: params} do
-      {:ok, %{raw: raw}} = ChunkExtractor.run(%{params | direction: "down"})
+      {:ok, %{raw: raw}} = ChunkExtractor.run(%{params | from_line: 1, to_line: 2})
       assert "foo 1\nbar 2\n" = raw
     end
 
-    test "reads first 2 lines up", %{params: params} do
-      {:ok, %{raw: raw}} = ChunkExtractor.run(%{params | direction: "up", from_line: 2})
-      assert "foo 1\nbar 2" = raw
-    end
-
     test "error when file doesn't exist", %{params: params} do
       {:error, %{errors: errors}} = ChunkExtractor.run(%{params | file_path: "non_existent"})
       assert Keyword.get(errors, :system) =~ ~r/non_existent: No such file/
     end
 
-    test "error when arguments are not valid", %{params: params} do
-      {:error, %{errors: errors}} = ChunkExtractor.run(%{params | from_line: -1})
-      assert Keyword.get(errors, :system) =~ ~r/illegal offset/
+    test "returns from 1 when from_line is negative", %{params: params} do
+      {:ok, %{parsed: actual}} =
+        ChunkExtractor.run(%{params | from_line: -2, right_line: -2, to_line: 2})
+
+      assert [
+               %{from_line_number: 1, to_line_number: 1},
+               %{from_line_number: 2, to_line_number: 2}
+             ] = actual
     end
 
-    test "reads 2 lines up from the middle", %{params: params} do
-      {:ok, %{raw: raw}} = ChunkExtractor.run(%{params | direction: "up", from_line: 3})
-      assert "bar 2\nbaz 3" = raw
+    test "error when arguments are not valid", %{params: params} do
+      {:error, %{errors: errors}} = ChunkExtractor.run(%{params | from_line: -4, to_line: -3})
+      assert Keyword.get(errors, :param) == "from_line parameter must be less than to_line"
     end
 
-    test "reads 2 lines down from the middle", %{params: params} do
-      {:ok, %{raw: raw}} = ChunkExtractor.run(%{params | direction: "down", from_line: 2})
-      assert "bar 2\nbaz 3\n" = raw
+    test "reads 2 lines from the middle", %{params: params} do
+      {:ok, %{raw: raw}} = ChunkExtractor.run(%{params | from_line: 2, to_line: 4})
+      assert "bar 2\nbaz 3\nbaf 4\n" = raw
     end
   end
 
   describe "parse_chunk" do
     test "parses raw chunk into list of structs", %{params: params} do
       {:ok, %{parsed: actual}} = ChunkExtractor.run(params)
-      assert [%{line_text: "foo 1"}, %{line_text: "bar 2"}] = actual
+      assert [%{text: " foo 1"}, %{text: " bar 2"}] = actual
     end
 
-    test "sets line_numbers when direction is down", %{params: params} do
-      {:ok, %{parsed: actual}} = ChunkExtractor.run(%{params | direction: "down", from_line: 2})
-      assert [%{line_number: 2}, %{line_number: 3}] = actual
-    end
+    test "sets line_numbers", %{params: params} do
+      {:ok, %{parsed: actual}} =
+        ChunkExtractor.run(%{params | from_line: 2, right_line: 1, to_line: 3})
 
-    test "sets line_numbers when direction is up", %{params: params} do
-      {:ok, %{parsed: actual}} = ChunkExtractor.run(%{params | direction: "up", from_line: 3})
-      assert [%{line_number: 2}, %{line_number: 3}] = actual
+      assert [
+               %{from_line_number: 2, to_line_number: 1},
+               %{from_line_number: 3, to_line_number: 2}
+             ] = actual
     end
   end
 end