testscript: add env convenience functions #96
Merged
Conversation
This file contains 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
rogpeppe
requested changes
May 5, 2020
testscript/testscript.go
Outdated
| // If the variable is present in the environment the value (which may be empty) | ||
| // is returned and the boolean is true. Otherwise the returned value will be | ||
| // empty and the boolean will be false. | ||
| func (e *Env) LookupEnv(key string) (value string, found bool) { |
There was a problem hiding this comment.
I'm wondering about the use case for these methods. In general we're not working with an arbitrary set of environment variables here. I don't really see the use case for LookupEnv - it didn't even exist in the standard library for ages. How about just providing Getenv and Setenv?
testscript/testscript.go
Outdated
| // empty and the boolean will be false. | ||
| func (e *Env) LookupEnv(key string) (value string, found bool) { | ||
| key = envvarname(key) | ||
| for _, v := range e.Vars { |
There was a problem hiding this comment.
how about iterating in reverse just to make things a bit clearer?
for i := len(e.Vars)-1; i >= 0; i-- {
if pair := strings.SplitN(v, "=", 2); len(pair) == 2 && envvarname(pair[0]) == key {
return pair[1]
}
}
return ""
| // Setenv sets the value of the environment variable named by the key. It | ||
| // returns an error, if any. | ||
| func (e *Env) Setenv(key, value string) error { | ||
| if key == "" || strings.IndexByte(key, '=') != -1 { |
There was a problem hiding this comment.
This isn't the os package. I'd suggest just panicking in this case, which avoids the need to decide what kind of error to return here (I'm not that keen on the syscall import).
|
Thanks for the review. All comments addressed and PR updated. |
twpayne
force-pushed
the
testscript-env
branch
2 times, most recently
from
2141521
to
1137e5e
Compare
Manipulating Env.Vars is not completely trivial: you need to handle last-entry-wins, invalid entries, and key normalization. This commit adds Getenv and Setenv method to Env that function identically to their counterparts in the standard os package.
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
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.
Manipulating
Env.Varsis not completely trivial: you need to handle last-entry-wins, invalid entries, and key normalization.This PR adds
Getenv,LookupEnv, andSetenvmethod toEnvthat function identically to their counterparts in the standardospackage.Example use: