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

Get dependency command #2

Open
wants to merge 2 commits into
base: master
Choose a base branch
from
Open

Conversation

rudeqit
Copy link

@rudeqit rudeqit commented Jan 27, 2022

I suggest minor improvements:

  1. Remove self_hash from rebar.config.save file
  2. Add get command for pulling deps from save file(not just checkout like load)
  3. Up and get single deps

};
{error, enoent} -> {"deps", []}
end,
DepsFldr = deps_dir(RebarCfg),
Copy link
Owner

Choose a reason for hiding this comment

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

Так dir или fldr? думаю лучше все же dir чтобы было и слева и справа

DepsList).

-spec deps_list(RebarCfg :: string()) -> DepsList :: list().
deps_list(RebarCfg) ->
Copy link
Owner

Choose a reason for hiding this comment

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

две очень похожие функции, почти копипаст + дважды читается с диска
Может сделать одну функцию которая оба значения вернет и список приложений и директорию?

foreach/5,
foreach/6
]).
-export([foreach/4, foreach/5, foreach/6, deps_list/1, deps_map/1, deps_dir/1]).
Copy link
Owner

Choose a reason for hiding this comment

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

deps_list где то извне используется разве?

deps_list(RebarCfg) ->
case file:consult(RebarCfg) of
{ok, Config} ->
proplists:get_value(deps, Config, []);
Copy link
Owner

Choose a reason for hiding this comment

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

сам по себе список нигде не нужен, везде используется map
так может тут сделать list comprehension + maps:from_list?

?WARN("Drop ~p", [Drop]),
Acc
end, {queue:new(), gb_sets:new()}, DepsList),
{Q, ViewedDeps} =
Copy link
Owner

Choose a reason for hiding this comment

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

А какие тут функциональные изменения?
если что то не нравится по стилю пожелейте мейнтейнера, отправте отдельным PR ну или хотя бы отдельным комитом внутри

(_, Acc) ->
Acc
end, {DownloadedList, AccResult}, CorrectDownList),
bfs_step(Module, Dir, Q, ViewedDeps, DepsList, gb_sets:new(), AccResult, Delay, Args).
Copy link
Owner

Choose a reason for hiding this comment

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

elvis не запускали?
тут вроде более 80 символов, перечит code style проекта

Acc
end,
{DownloadedList, AccResult},
CorrectDownList),
Copy link
Owner

Choose a reason for hiding this comment

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

Тоже кусок без изменений как я понял?


case maps:find(App, Deps) of
{ok, Source} ->
VSN = "", % TODO
Copy link
Owner

Choose a reason for hiding this comment

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

TODO что?

@@ -39,7 +72,8 @@ do(Dir, App, _VSN, Source, [Fast], _IsVerbose) ->
end.

update_source(App, AppDir, URL) ->
Cmd = "git --no-pager log --branches --not --remotes --quiet --pretty=tformat:'%h %an %s'",
Cmd = "git --no-pager log --branches --not --remotes --quiet --pretty=tform"
Copy link
Owner

Choose a reason for hiding this comment

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

Попрошу разбить на строки хотя бы по словам, сложно читать иначе

@platinumthinker
Copy link
Owner

Если хотите быстрее влить, выкинте все не функциональные изменения (по форматингу кода) и будет всем счастье

@platinumthinker
Copy link
Owner

По существу:

  1. Не совсем понятно какую цель приследуете этим и чем мешает он?
  2. Отличная функциональность, но кажется get:do и updater:do очень похожи, нельзя ли переиспользовать один do в другом?
  3. Тоже хорошая опция

@platinumthinker platinumthinker self-assigned this Apr 27, 2022
@platinumthinker
Copy link
Owner

Судя по всему изменений не дождаться. Буду сам гранулярно в мастер перетаскивать то с чем согласен (save, get)

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.

2 participants