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

Fix path command to consider {deps_dir, "deps"} in rebar.config #1197

Merged
merged 1 commit into from
May 18, 2016

Conversation

zsoci
Copy link

@zsoci zsoci commented May 17, 2016

No description provided.

lib_dir(State) -> io_lib:format("~s/lib", [rebar_dir:base_dir(State)]).
lib_dir(State) -> io_lib:format("~s/~s", [rebar_dir:base_dir(State),
rebar_state:get(State, deps_dir,
?DEFAULT_DEPS_DIR)]).
Copy link
Collaborator

Choose a reason for hiding this comment

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

Rather than using this format, consider calling rebar_dir:deps_dir/1 for all of this patch. It seems that using rebar_dir:base_dir/1 was just an oversight.

@ferd
Copy link
Collaborator

ferd commented May 17, 2016

I'm okay with the objective and patching of this PR, but believe the implementation should reuse rebar_dir:deps_dir/1 rather than re-implementing similar code in multiple places in the file. Once that change is done, this seems good for me.

@zsoci
Copy link
Author

zsoci commented May 18, 2016

Good point. Fixed. Thanks.

@ferd
Copy link
Collaborator

ferd commented May 18, 2016

You only changed it for 1/4 of the lines there. The same comment applies to all 3 others. Sorry I wasn't clear enough.

@zsoci
Copy link
Author

zsoci commented May 18, 2016

Fixed, common tests are ok locally, no idea why failed here.

@ferd
Copy link
Collaborator

ferd commented May 18, 2016

Some test that apparently fails from time to time. Thanks for the contribution!

@ferd ferd merged commit 41addaa into erlang:master May 18, 2016
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