Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Feature: expand context #53

Open
wants to merge 10 commits into
base: main
Choose a base branch
from
21 changes: 21 additions & 0 deletions assets/css/app.css
Original file line number Diff line number Diff line change
Expand Up @@ -198,8 +198,29 @@ input.search-input:focus {
display: none;
}

.ghd-chunk-header {
height: 44px;
}
.ghd-chunk-header:first-child, .ghd-chunk-header:last-child {
height: 0;
}

.ghd-chunk-header .ghd-line-number {
background-color: #f8fafd;
flex-direction: column;
}

.expanded, .expanded .ghd-line-number{
background-color: #f5f5f5;
}

.ghd-chunk-header .ghd-line-number div{
background: #dbedff;
width: 100%;
text-align: center;
}
.ghd-chunk-header .ghd-line-number div:hover{
background: #95c6fe;
}

.ghd-file-status {
Expand Down
74 changes: 74 additions & 0 deletions assets/js/app.js
Original file line number Diff line number Diff line change
Expand Up @@ -57,3 +57,77 @@ fileHeaders.forEach(header => {
const scrollIfNeeded = elem => {
elem.getBoundingClientRect().top < 0 && elem.scrollIntoView(true)
}

const MAX_EXPAND_CONTEXT_LINES = 20

document.addEventListener('DOMContentLoaded', e => {
document.querySelectorAll('.ghd-expand-up').forEach(expandUp => {
expandUp.addEventListener('click', e => {
const {fileName, packageName, version, lineAfter, lineBefore, patch} = gatherInfo(expandUp)

// expandUp always follows by diff line.. so we take the number
const toLine = numberFrom(lineAfter) - 1

const _fromLine = lineBefore ? numberFrom(lineBefore) + 1 : 1
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Let's avoid the underscored variable names. Please pick a different name or don't use const.

const fromLine = Math.max(toLine - MAX_EXPAND_CONTEXT_LINES, _fromLine)

const _rightLine = lineBefore ? numberTo(lineBefore) + 1 : 1
const rightLine = Math.max(toLine - MAX_EXPAND_CONTEXT_LINES, _rightLine)

fetchChunkAndInsert({target: lineAfter, packageName, version, fromLine, toLine, rightLine, fileName, patch})
})
})

document.querySelectorAll('.ghd-expand-down').forEach(expandDown => {
expandDown.addEventListener('click', e => {
const {fileName, packageName, version, lineAfter, lineBefore, patch} = gatherInfo(expandDown)

const fromLine = numberFrom(lineBefore) + 1
const rightLine = numberTo(lineBefore) + 1

const _toLine = lineAfter ? numberFrom(lineAfter) - 1 : Infinity
const toLine = Math.min(fromLine + MAX_EXPAND_CONTEXT_LINES, _toLine)

fetchChunkAndInsert({target: expandDown.closest('tr'), packageName, version, fromLine, toLine, rightLine, fileName, patch})

})
})
})

const numberFrom = line => parseInt(line.querySelector('.ghd-line-number .ghd-line-number-from').textContent.trim())
const numberTo = line => parseInt(line.querySelector('.ghd-line-number .ghd-line-number-to').textContent.trim())

const gatherInfo = line => {
const patch = line.closest('.ghd-file')
const {fileName, packageName, version} = patch.querySelector('.ghd-file-header').dataset

const lineAfter = line.closest('tr').nextElementSibling
const lineBefore = line.closest('tr').previousElementSibling

return {fileName, packageName, version, lineAfter, lineBefore, patch}
}

const fetchChunkAndInsert = params => {
if( !(params.fromLine && params.toLine) ||
(params.fromLine >= params.toLine) ){
RudolfMan marked this conversation as resolved.
Show resolved Hide resolved
return
}

const path = `/diff/${params.packageName}/${params.version}/expand/${params.fromLine}/${params.toLine}/${params.rightLine}`
const url = new URL(path, window.location)
url.searchParams.append('file_name', params.fileName)

fetch(url)
.then(response => response.json())
.then(({chunk, lines, errors}) => {
if(errors){return}
RudolfMan marked this conversation as resolved.
Show resolved Hide resolved
const context = document.createElement('template')
context.innerHTML = chunk.trim()
const patchBody = params.patch.querySelector('tbody')

Array.prototype.reduceRight.call(context.content.childNodes, (target, line) => {
return patchBody.insertBefore(line, target)
}, params.target)
})
.catch(console.error)
}
112 changes: 112 additions & 0 deletions lib/diff/hex/chunk_extractor.ex
Original file line number Diff line number Diff line change
@@ -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]
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

We can use a map internally and only return the parsed part, we don't need the struct.


def run(params) do
params
|> cast()
|> parse_integers([:right_line, :from_line, :to_line])
|> normalize([:from_line, :right_line])
|> validate_to_line()
|> system_read_raw_chunk()
|> parse_chunk()
|> case do
%{errors: []} = chunk -> {:ok, chunk}
%{errors: _} = chunk -> {:error, chunk}
end
end
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Instead of having every function match on %{errors: ...} use with instead.


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
Enum.reduce(fields, chunk, &parse_integer/2)
end

defp parse_integer(field, chunk) do
value = chunk |> Map.get(field) |> parse_value()

case value do
:error -> %{chunk | errors: [{:parse_integer, "#{field} must be a number"} | chunk.errors]}
integer -> Map.put(chunk, field, integer)
end
end

defp parse_value(number) when is_integer(number), do: number

defp parse_value(number) when is_binary(number) do
with {int, _} <- Integer.parse(number), do: int
end

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_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
path = chunk.file_path
from_line = to_string(chunk.from_line)
to_line = to_string(chunk.to_line)

case System.cmd("sed", ["-n", "#{from_line},#{to_line}p", path], stderr_to_stdout: true) do
{raw_chunk, 0} ->
%{chunk | raw: raw_chunk}

{error, code} ->
error = {:system, "System command exited with a non-zero status #{code}: #{error}"}
%{chunk | errors: [error | chunk.errors]}
end
end

defp parse_chunk(%{errors: [_ | _]} = chunk), do: chunk

defp parse_chunk(chunk) do
parsed =
chunk.raw
|> String.split("\n")
|> 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 remove_trailing_newline(lines) do
lines
|> Enum.reverse()
|> tl()
|> Enum.reverse()
end
end
35 changes: 34 additions & 1 deletion lib/diff/hex/hex.ex
Original file line number Diff line number Diff line change
Expand Up @@ -39,9 +39,18 @@ defmodule Diff.Hex do
end

def unpack_tarball(tarball, path) when is_binary(path) do
do_unpack_tarball(tarball, :all_files, path)
end

def unpack_tarball(tarball, file_list, path) when is_binary(path) do
file_list = Enum.map(file_list, &to_charlist/1)
do_unpack_tarball(tarball, file_list, path)
end

def do_unpack_tarball(tarball, file_list, path) do
path = to_charlist(path)

with {:ok, _} <- :hex_tarball.unpack(tarball, path) do
with {:ok, _} <- :hex_tarball.unpack(tarball, file_list, path) do
:ok
end
end
Expand Down Expand Up @@ -100,6 +109,30 @@ defmodule Diff.Hex do
end
end

def get_chunk(params) do
path = tmp_path("chunk")

chunk_extractor_params = Map.put(params, :file_path, Path.join(path, params.file_name))

try do
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
{:error, %Diff.Hex.ChunkExtractor{errors: errors} = chunk} ->
Logger.error(inspect(errors))
{:error, chunk}

error ->
Logger.error(inspect(error))
{:error, error}
end
after
File.rm_rf(path)
end
end

defp git_diff(path_from, path_to, path_out) do
case System.cmd("git", [
"-c",
Expand Down
51 changes: 50 additions & 1 deletion lib/diff_web/controllers/page_controller.ex
Original file line number Diff line number Diff line change
Expand Up @@ -28,6 +28,27 @@ defmodule DiffWeb.PageController do
end
end

def expand_context(conn, %{"file_name" => _file_name} = params) do
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Is file_name the only required parameter?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

case parse_version(params["version"]) do
{:ok, version} ->
version = to_string(version)
params = Map.update!(params, "version", fn _ -> version end)

do_expand_context(conn, params)

:error ->
conn
|> put_status(400)
|> json(%{error: "Bad Request"})
end
end

def expand_context(conn, _params) do
conn
|> put_status(400)
|> json(%{error: "missing query parameter: file_name"})
end

defp maybe_cached_diff(conn, _package, version, version) do
render_error(conn, 400)
end
Expand Down Expand Up @@ -70,6 +91,30 @@ defmodule DiffWeb.PageController do
end
end

defp do_expand_context(conn, params) do
chunk_extractor_params =
for {key, val} <- params, into: %{} do
{String.to_existing_atom(key), val}
end

case Diff.Hex.get_chunk(chunk_extractor_params) do
{:ok, chunk} ->
rendered_chunk =
Phoenix.View.render_to_string(DiffWeb.RenderView, "render_context_chunk.html",
chunk: chunk
)
RudolfMan marked this conversation as resolved.
Show resolved Hide resolved

conn
|> put_status(200)
|> json(%{chunk: rendered_chunk, lines: length(chunk)})

{:error, %{errors: errors}} ->
conn
|> put_status(400)
|> json(%{errors: Enum.into(errors, %{})})
end
end

defp do_diff(conn, package, from, to) do
case Diff.Hex.diff(package, from, to) do
{:ok, stream} ->
Expand Down Expand Up @@ -134,7 +179,11 @@ defmodule DiffWeb.PageController do
Enum.each(stream, fn
{:ok, patch} ->
html_patch =
Phoenix.View.render_to_iodata(DiffWeb.RenderView, "render.html", patch: patch)
Phoenix.View.render_to_iodata(DiffWeb.RenderView, "render.html",
patch: patch,
package: package,
from_version: from
)

IO.binwrite(file, html_patch)

Expand Down
5 changes: 5 additions & 0 deletions lib/diff_web/router.ex
Original file line number Diff line number Diff line change
Expand Up @@ -15,6 +15,11 @@ defmodule DiffWeb.Router do
pipe_through :browser

live "/", SearchLiveView

get "/diff/:package/:version/expand/:from_line/:to_line/:right_line/",
PageController,
:expand_context

get "/diff/:package/:versions", PageController, :diff
get "/diffs", PageController, :diffs
end
Expand Down
18 changes: 14 additions & 4 deletions lib/diff_web/templates/render/render.html.eex
Original file line number Diff line number Diff line change
@@ -1,6 +1,6 @@
<% status = patch_status(@patch) %>
<div class="ghd-file">
<div class="ghd-file-header">
<div class="ghd-file-header" data-file-name="<%= @patch.from %>" data-package-name="<%= @package %>" data-version="<%= @from_version %>" >
<span class="ghd-file-status ghd-file-status-<%= status %>">
<%= status %>
</span>
Expand All @@ -10,11 +10,13 @@
</div>
<div class="ghd-diff">
<table class="ghd-diff">
<%= for chunk <- @patch.chunks do %>
<%= for {chunk, index} <- Enum.with_index(@patch.chunks) do %>
<tr class="ghd-chunk-header">
<td class="ghd-line-number">
<div class="ghd-line-number-from">&nbsp;</div>
<div class="ghd-line-number-to"></div>
<%= unless index == 0 do %>
<div class="ghd-expand-down"><svg class="octicon octicon-fold-down" viewBox="0 0 14 16" version="1.1" width="14" height="16" aria-hidden="true"><path fill-rule="evenodd" d="M4 11l3 3 3-3H8V5H6v6H4zm-4 0c0 .55.45 1 1 1h2.5l-1-1h-1l2-2H5V8H3.5l-2-2H5V5H1c-.55 0-1 .45-1 1l2.5 2.5L0 11zm10.5-2H9V8h1.5l2-2H9V5h4c.55 0 1 .45 1 1l-2.5 2.5L14 11c0 .55-.45 1-1 1h-2.5l1-1h1l-2-2z"></path></svg></div>
<% end %>
<div class="ghd-expand-up"><svg xmlns="http://www.w3.org/2000/svg" viewBox="0 0 14 16" width="14" height="16"><path fill-rule="evenodd" d="M10 6L7 3 4 6h2v6h2V6h2zm4 0c0-.55-.45-1-1-1h-2.5l1 1h1l-2 2H9v1h1.5l2 2H9v1h4c.55 0 1-.45 1-1l-2.5-2.5L14 6zM3.5 8H5v1H3.5l-2 2H5v1H1c-.55 0-1-.45-1-1l2.5-2.5L0 6c0-.55.45-1 1-1h2.5l-1 1h-1l2 2z"></path></svg></div>
</td>
<td class="ghd-text">
<div class="ghd-text-internal"><%= chunk.header %></div>
Expand All @@ -36,6 +38,14 @@
</tr>
<% end %>
<% end %>
<tr class="ghd-chunk-header">
<td class="ghd-line-number">
<div class="ghd-expand-down"><svg class="octicon octicon-fold-down" viewBox="0 0 14 16" version="1.1" width="14" height="16" aria-hidden="true"><path fill-rule="evenodd" d="M4 11l3 3 3-3H8V5H6v6H4zm-4 0c0 .55.45 1 1 1h2.5l-1-1h-1l2-2H5V8H3.5l-2-2H5V5H1c-.55 0-1 .45-1 1l2.5 2.5L0 11zm10.5-2H9V8h1.5l2-2H9V5h4c.55 0 1 .45 1 1l-2.5 2.5L14 11c0 .55-.45 1-1 1h-2.5l1-1h1l-2-2z"></path></svg></div>
</td>
<td class="ghd-text">
<div class="ghd-text-internal"></div>
</td>
</tr>
</table>
</div>
</div>
Loading