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

[UPGRADE] Upgrade functions call that remove warnings #8

Merged
merged 2 commits into from
Oct 29, 2021

Conversation

GuillaumeMilan
Copy link

Issue #7

Copy link
Member

@Shakadak Shakadak left a comment

Choose a reason for hiding this comment

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

Substitutions of "?" by "/" are necessary, others comments can be discussed.

lib/gitex.ex Outdated
@@ -40,7 +40,7 @@ defmodule Gitex do
def get_hash(ref,repo,path), do: get_hash(get(ref,repo),repo,path)

def get_hash(hash,tree,repo,path), do:
get_hash_path(hash,tree,repo,path |> String.strip(?/) |> String.split("/"))
get_hash_path(hash,tree,repo,path |> String.trim("?") |> String.split("/"))
Copy link
Member

Choose a reason for hiding this comment

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

Here you'll need to replace "?" with "/"

lib/gitex.ex Outdated
@@ -51,7 +51,7 @@ defmodule Gitex do
def put(%{tree: tree},repo,path,elem), do: put(object(tree,repo),repo,path,elem)
def put(%{object: ref},repo,path,elem), do: put(object(ref,repo),repo,path,elem)
def put(tree,repo,path,elem) when is_list(tree), do:
({:tree,ref}=put_path(tree,repo,path |> String.strip(?/) |> String.split("/"),elem); ref)
({:tree,ref}=put_path(tree,repo,path |> String.trim("?") |> String.split("/"),elem); ref)
Copy link
Member

Choose a reason for hiding this comment

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

Same stuff here, replace "?" with "/"

@@ -239,8 +239,8 @@ defmodule Gitex.Git do
defp apply_delta_hunks(acc,base,<<0::size(1),add_len::size(7),add::binary-size(add_len)>> <> delta), do:
apply_delta_hunks([acc,add],base,delta)
defp apply_delta_hunks(acc,base,<<1::size(1),len_shift::bitstring-size(3),off_shift::bitstring-size(4)>> <> delta) do
off_ops = Enum.zip([0,8,16,24],Enum.reverse(for(<<x::size(1)<-off_shift>>,do: x))) |> Enum.filter_map(& elem(&1,1)==1,& elem(&1,0))
len_ops = Enum.zip([0,8,16],Enum.reverse(for(<<x::size(1)<-len_shift>>,do: x))) |> Enum.filter_map(& elem(&1,1)==1,& elem(&1,0))
off_ops = Enum.zip([0,8,16,24],Enum.reverse(for(<<x::size(1)<-off_shift>>,do: x))) |> Stream.filter(& elem(&1,1)==1) |> Enum.map(& elem(&1,0))
Copy link
Member

Choose a reason for hiding this comment

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

Is the Stream usage necessary ? There is overhead in the construction and evaluation of a Stream, and I prefer to use Enum unless there is a tangible benefit.

Copy link
Author

Choose a reason for hiding this comment

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

I don't know

readme: "README.md", main: "README",
source_url: "https://github.com/awetzel/gitex",
source_ref: "master"
],
package: package(),
description: description(),
deps: [{:ex_doc, ">= 0.0.0", only: :dev}]]
elixirc_options: [warnings_as_errors: true],
deps: [{:ex_doc, "~> 0.19", only: :dev, runtime: false}]]
Copy link
Member

Choose a reason for hiding this comment

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

Any reason for 0.19 instead of a more up to date version ?

Copy link
Author

Choose a reason for hiding this comment

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

How sweet. It was change automatically by something but I can't figure out what.
If fix this too

end

def application do
[applications: [:logger],
[extra_applications: [:crypto],
Copy link
Member

Choose a reason for hiding this comment

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

Can you explain why :crypto is needed but not :logger in the extra_applications

Copy link
Author

Choose a reason for hiding this comment

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

The application depends on the crypto application as it uses some of its functions.
But the application doesn't use any logger any where.
A good addition would be to log some stuff too.

@GuillaumeMilan GuillaumeMilan merged commit 4312fc7 into kbrw:master Oct 29, 2021
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

2 participants