Feature: expand context #53
Conversation
add chunk.sh shell script that returns cut of the file based on starting line, number of lines to read and direction (up or down) add ChunkExtractor module that sanitizes params and handles response of the chunk.sh script add a json endpoint to be able to request chunks
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
…o render the chunk
| @@ -43,7 +43,7 @@ defmodule Diff.MixProject do | |||
| {:jason, "~> 1.0"}, | |||
| {:plug_cowboy, "~> 2.0"}, | |||
| {:phoenix_live_view, "~> 0.6"}, | |||
| {:hex_core, "~> 0.6.1"}, | |||
| {:hex_core, github: "RudolfMan/hex_core", branch: "unpack-list-of-files"}, | |||
There was a problem hiding this comment.
PR for the branch hexpm/hex_core#95
…m :all_files instead of empty list for unpacking all files)
RudolfMan
force-pushed
the
expand-context
branch
from
14a8cc8
to
e85ddc5
Compare
|
Thanks for the PR, it looks really great. Just a few points that I think we can improve: Fetching a tarball and unpacking it every time a section expanded is a bit wasteful. It would be better to upload the individual files to the bucket after the tarball is unpacked for diffing and when we expand only fetch that file instead of the whole tarball. The expand buttons should not be displayed when there is nothing to expand:
I am also a bit concerned about the amount of javascript, but I am not sure how well liveview would work on the potentially large diffs we show. If we start using liveview I think it would be worth showing smaller diffs and ask users to expand the diff to show more files. For now the javascript is fine but if you want to try out liveview for this page in a separate PR then go ahead :). |
| // expandUp always follows by diff line.. so we take the number | ||
| const toLine = numberFrom(lineAfter) - 1 | ||
|
|
||
| const _fromLine = lineBefore ? numberFrom(lineBefore) + 1 : 1 |
There was a problem hiding this comment.
Let's avoid the underscored variable names. Please pick a different name or don't use const.
| @@ -0,0 +1,112 @@ | |||
| defmodule Diff.Hex.ChunkExtractor do | |||
| @enforce_keys [:file_path, :from_line, :to_line, :right_line] | |||
| defstruct Enum.map(@enforce_keys, &{&1, nil}) ++ [errors: [], raw: nil, parsed: nil] | |||
There was a problem hiding this comment.
We can use a map internally and only return the parsed part, we don't need the struct.
| %{errors: []} = chunk -> {:ok, chunk} | ||
| %{errors: _} = chunk -> {:error, chunk} | ||
| end | ||
| end |
There was a problem hiding this comment.
Instead of having every function match on %{errors: ...} use with instead.
| @@ -28,6 +28,27 @@ defmodule DiffWeb.PageController do | |||
| end | |||
| end | |||
|
|
|||
| def expand_context(conn, %{"file_name" => _file_name} = params) do | |||
There was a problem hiding this comment.
Is file_name the only required parameter?
There was a problem hiding this comment.
Actually since file_name is required parameter - makes sense to add it to "path" rather that "query_params" that way we won't need to handle that missing file_name in the controller. Additionally, I see some other parameters might be not strictly required and pronbably makes sense to have them in query params..
I'll revisit this part
Co-authored-by: Eric Meadows-Jönsson <[email protected]>
The ability to expand the context of the code around the change.
There are definitely stuff to improve.. especially in my frontend spaghetti JS (not sure if it makes sense to rewrite Diffs views to live_view since that page isn't meant to be live)
So currently onclick on any button "expanUp" or "expandDown" script tries to read numbers from lines before and after the button and makes the request:
GET /diff/:package/:version/expand/:from_line/:to_line/:right_line?file_name=<file_name>In the backend we unpack that file from, extract the requested chunk and render the html
To be able to unpack particular file I propose a patch to
hex_coreto add:hex_tarball.unpack/3to accept the list of files. see hexpm/hex_core#95