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

Update home usage to new env fn #3073

Merged
merged 3 commits into from
Oct 13, 2022

Conversation

LucioFranco
Copy link
Member

This is the corresponding PR for brson/home#29 which updates how rustup uses the home crates Env features for testing.

This is still a draft PR pending a new release of the home crate with the new features.

cc @kinnison

@rbtcollins
Copy link
Contributor

Sure, looks fine I guess - aesthetically it has stuttering. I realise this isn't a rust style guide issue but it is one that I find very helpful in making nice APIs. (And not the extreme version presented in some golang talks!).

But not having to maintain home is much more beneficial than a few extra env:s here and there.

That said, I think this would be a smaller patch if instead of the insertion of ::env you added use home::env as home at the top of each affected source file.

@LucioFranco
Copy link
Member Author

That said, I think this would be a smaller patch if instead of the insertion of ::env you added use home::env as home at the top of each affected source file.

Oh that is a great idea, I didn't think of that, will make that change.

Copy link
Contributor

@kinnison kinnison left a comment

Choose a reason for hiding this comment

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

As it stands, I think that this is clean and clear. Once a new home release is done, I look forward to this being updated for the new home version.

@LucioFranco
Copy link
Member Author

Hm I feel like the ring failure has nothing to do with my changes?

@LucioFranco LucioFranco marked this pull request as ready for review October 11, 2022 14:58
@hi-rustin
Copy link
Member

Hm I feel like the ring failure has nothing to do with my changes?

Yes. See: rust-lang/rust#102332 (comment)

Copy link
Member

@hi-rustin hi-rustin left a comment

Choose a reason for hiding this comment

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

Thanks!

@hi-rustin hi-rustin merged commit 732feb8 into rust-lang:master Oct 13, 2022
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

4 participants