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

Profiles #31

Merged
merged 18 commits into from
Dec 2, 2014
Merged

Profiles #31

merged 18 commits into from
Dec 2, 2014

Conversation

tsloughter
Copy link
Collaborator

@tsloughter tsloughter changed the title WIP: Profiles Profiles Nov 29, 2014
apply_profile(State=#state_t{opts=Opts}, Profile) ->
ConfigProfiles = rebar_state:get(State, profiles, []),
Deps = rebar_state:get(State, deps, []),
Opts1 = dict:store({deps, default}, Deps, dict:erase(deps, Opts)),
Copy link
Collaborator

Choose a reason for hiding this comment

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

This will only work once, assuming the current profile is default. The next time you call this, you'll possibly corrupt your default state.

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

Hm, you are right. Question then is, should you be able to apply multiple profiles. lein supports that, I originally planned to simplify it by sticking to only allowing default + other profile.

So I guess apply_profile/2 should reset to default and then apply any profile. So default profile will be cached in the state somewhere.

@tsloughter
Copy link
Collaborator Author

@ferd ok, I'm stopping adding to this PR :). Just pushed dialyzer fixes so I think it is ready. Let me know.

Note, I weakened some specs in rebar_prv_install_deps that I want to strengthen later and that whole module could use another run though of potential refactoring and commenting. I'm debating adding a 'state' record to that provider as well so so many variables don't have to be passed around to each function.

@ferd
Copy link
Collaborator

ferd commented Dec 1, 2014

That's a huge huge branch with intertwined fixes so I'm gonna go through the diff to try to see if I get things right, but some of my comments would have been caught by reading commits individually.

@@ -220,7 +220,7 @@ And then we can implement the switch to figure out what to search:
do(State) ->
Apps = case discovery_type(State) of
project -> rebar_state:project_apps(State);
deps -> rebar_state:project_apps(State) ++ rebar_state:src_deps(State)
deps -> rebar_state:project_apps(State) ++ rebar_state:all_deps(State)
Copy link
Collaborator

Choose a reason for hiding this comment

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

I'm seeing why it's being done that way, but do the binary packages ship with the source code? Otherwise the demo app doing a search over source might look funny. Also this should require a patch on https://bitbucket.org/ferd/rebar3-todo-plugin to match the tutorial.

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

They do ship with source code. And yea, I'd have sent a PR if it was on github :P

Copy link
Collaborator

Choose a reason for hiding this comment

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

Yeah we should just take a note when merging it so that I can go and update it.

@tsloughter
Copy link
Collaborator Author

Pushed some fixes. @ferd when you get a chance let me know if I can merge this.

@ferd
Copy link
Collaborator

ferd commented Dec 2, 2014

I believe you forgot to push rebar_dir in your last commit @tsloughter

@tsloughter
Copy link
Collaborator Author

Yup, pushed.

tsloughter added a commit that referenced this pull request Dec 2, 2014
@tsloughter tsloughter merged commit 3b2d9ba into erlang:master Dec 2, 2014
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