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

Add mockable home fn and Env trait #29

Merged
merged 6 commits into from Oct 10, 2022
Merged

Add mockable home fn and Env trait #29

merged 6 commits into from Oct 10, 2022

Conversation

LucioFranco
Copy link
Collaborator

This PR follows up and replaces on #23 which intended to add mockable
functions to the home crate. This PR follows in similar vain except
for a few changes.

  • All mockable fn and the Env trait moved into a new env module to
    keep the docs clean from all the extra methods that are usually not
    required for most users of the crate.
  • Rename the functions to be with_env instead of just having _from.

The goal of this PR is to enable rustup and hopefully cargo in the
future to be able to write tests which mockout the environment while
still going through the core logic of this crate.

cc @rbtcollins @kinnison

rbtcollins and others added 5 commits September 19, 2022 15:11
These permit abstracting over some common global process state and allow
the use of home in project where in-process threaded tests are
desirable, and where rustup / cargo state is changed during the tests.
This needs to be public to allow rustup to cleanly call the from family
of functions but still benefit from the Windows specific codepath.
This PR follows up and replaces on #23 which intended to add mockable
functions to the `home` crate. This PR follows in similar vain except
for a few changes.

- All mockable fn and the `Env` trait moved into a new `env` module to
    keep the docs clean from all the extra methods that are usually not
    required for most users of the crate.
- Rename the functions to be `with_env` instead of just having `_from`.

The goal of this PR is to enable `rustup` and hopefully `cargo` in the
future to be able to write tests which mockout the environment while
still going through the core logic of this crate.
@rbtcollins
Copy link
Contributor

I don't understand the changes vs what I did, and I don't have the time right now to build up context again to understand them - sorry.

But, if this permits the existing code in rustup, or equivalent code with different spelling, to work, then great, thank you.

@LucioFranco
Copy link
Collaborator Author

@rbtcollins sorry I could have explained that part better!

Basically, its your code but moved into a module and I renamed a few fn to what I personally found a bit easier to grok. So the actual technical changes are still the same. The reason I moved them into their own module is to make it so the main docs page is a bit lighter on fn in the docs and should make it easier for most users to find what they need but then reach into the env module to use the special fn + the trait you wrote to mock the diff std::env calls.

Hopefully that makes sense! Totally understand on the time part as well :) Thanks for checking in.

@LucioFranco LucioFranco merged commit de6dd85 into master Oct 10, 2022
@LucioFranco LucioFranco deleted the lucio/env branch October 10, 2022 13:24
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

3 participants