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

with_lib resets library path #836

Merged
merged 7 commits into from
Sep 2, 2015
Merged

with_lib resets library path #836

merged 7 commits into from
Sep 2, 2015

Conversation

krlmlr
Copy link
Member

@krlmlr krlmlr commented Jun 6, 2015

  • Reset library path after with_lib()
  • Includes test that fails without this modification

Closes #833, I'm planning to implement the with_temp_lib() function largely from scratch.

@hadley
Copy link
Member

hadley commented Jun 19, 2015

Could you please squash? (And close #833?)

@krlmlr
Copy link
Member Author

krlmlr commented Jul 3, 2015

Should be good to merge now. I have no idea what's wrong with the coverage -- I've added explicit tests. @jimhester Any idea?

@jimhester
Copy link
Member

It is failing the "patch coverage" test. Patch coverage refers to

The lines changed in each commit must meet this minimum coverage. Recommended value of 90%+.

https://github.com/codecov/support/wiki/Patch-Coverage-Status

Presumably because you did not add any new tests for the changed lines from this pull request. Looks like @hadley set it so that 90% of the changed lines need to have coverage to pass.

@krlmlr
Copy link
Member Author

krlmlr commented Jul 3, 2015

Thanks @jimhester for the quick reply. My point is:

@jimhester
Copy link
Member

Ah I misunderstood the issue, I will try to figure out what is going on
then.
On Jul 3, 2015 9:01 AM, "Kirill Müller" [email protected] wrote:

Thanks @jimhester https://github.com/jimhester for the quick reply. My
point is:


Reply to this email directly or view it on GitHub
#836 (comment).

@krlmlr
Copy link
Member Author

krlmlr commented Jul 3, 2015

Thanks. I probably should have been a bit more specific right from the start.

@jimhester
Copy link
Member

@krlmlr So the reason set_lib() was not being covered was because you were forcing the set and reset functions in with_something(). This forces a copy of the functions to be created, and only this copy was being run, not the original function. Removing those lines shows the proper coverage.

Kirill Müller added 2 commits August 15, 2015 18:36
forcing is probably unnecessary anyway
@krlmlr
Copy link
Member Author

krlmlr commented Aug 16, 2015

Removed the force() call as suggested by @jimhester and added a test to satisfy codecov.

hadley added a commit that referenced this pull request Sep 2, 2015
with_lib resets library path
@hadley hadley merged commit b11f66a into r-lib:master Sep 2, 2015
@krlmlr krlmlr deleted the with_lib_2 branch October 6, 2016 07:22
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.

3 participants