This repository was archived by the owner on Dec 29, 2021. It is now read-only.
-
Notifications
You must be signed in to change notification settings - Fork 19
Add with_env helper #46
Merged
Merged
Changes from all commits
Commits
File filter
Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
There are no files selected for viewing
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
There a reason we unconditionally clear and then re-add the
vars
we cleared viacompile
?There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
In fact we need the
env_clear()
because.envs
doesn't clear it or replace the whole variables.There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I think I'm missing something. I had assumed we'd only call
env_clear()
wheninherit == false
. Its unclear to me why we needenv_clear
wheninherit == true
.There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
By clearing the environment before adding our own environment, we don't have to check if inherit is true nor false in execute command. We just want to clear and add the appropriate environment. That's the environment role to know when to add the existing env or not.
If inherit is set to true, the
compile
method will return a merge of actual environment (env::vars()
) and our customized variables. I think it's nicer to have a compile method returning what our environment will look like. But feel free to propose something else I'm ok with it :)There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
tl;dr move along, these aren't the droids you're looking for.
That is nice. It helps with testing, etc.
I just assumed that
Assert
would directly access the members ofEnvironment
and there wouldn't be a need for a public.compile()
.Recreating an inherited environment is more work but I'm assuming it doesn't matter in this case, despite it being against "you pay for what you use" (adds cost to people not using these new features).
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@killercup how do you see this test?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Set an env var to something that is not UTF-8. How would I get a byte slice like that?
head -c 42 /dev/urandom | rcat --quote
(with this rcat) gives me:b"\xf4\xa9\xd5\x8d\xc0\xe8\x00\xff\x1dqz\x86k\x83\xcb~\xc1\x18!\xfa\x01J\x1c\\:\x18)%\xd4\xa6^7\xb4\x93\x80.\x9a\x9c\xe1\xcf\xe2,"
which is not valid UTF-8.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Yes, but
vars()
use UTF-8 variables, do we really want to support not valid UTF-8? I don't think..There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
That's exactly what I'm saying: I'm fine with only allowing our users to add UTF-8 vars but we should not destroy valid, inherited env vars. So, calling
vars
at all is probably not a good call, andvars_os
is the better option. Btw, you can find the impl of howstd::process::Command
deals with env vars on Unix here (it uses aHashMap<OsString, (usize, CString)>
to store env vars).How much work would it be to allow
OsStr
instead ofString
in our APIs?There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Can you still add a test with the non-UTF-8 env var value?