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

os: Getenv documentation update to point user to syscall.Getenv #9676

Closed
Stantheman opened this issue Jan 23, 2015 · 7 comments
Closed

os: Getenv documentation update to point user to syscall.Getenv #9676

Stantheman opened this issue Jan 23, 2015 · 7 comments
Assignees
Milestone

Comments

@Stantheman
Copy link
Contributor

@Stantheman Stantheman commented Jan 23, 2015

Quick version: I think it would be valuable to add a line of documentation to os.Getenv pointing the user to syscall.Getenv, if they want to check for the existence of an environment variable. I'd be happy to whip up the patch, or if someone is already in there, they can (if it's agreeable).

Long version: It looks like os.Getenv is a muddier version of syscall.Getenv. Specifically, it's syscall.Getenv, with an overloaded return that collides with possible real-world input. At first glance in the OS package, it looks like the only alternative for detecting the existence of an environment variable is to call os.Environ, but since it returns an array of strings as "KEY=VALUE" instead of a map, the user has to parse the results.

All os.Getenv does is call syscall.Getenv and throw away a value. The docs don't make it clear that syscall.Getenv would be a way better alternative to os.Environ, and it's unlikely the user would look up a (seemingly) unrelated package for a better option. The Go v1 release notes point the user to the right alternative. I think we should add a line to the docs to help point the user in the right direction, since the user will be less likely to have read those announcement notes as time goes on :).

The os.Getenv return has to stay the same for backwards-compatibility now, but I'll also argue for that to change in 2.0. That's for a separate issue though :)

@ianlancetaylor
Copy link
Contributor

@ianlancetaylor ianlancetaylor commented Jan 23, 2015

I'm reluctant to point people to the syscall package. I note that syscall.Getenv isn't even documented.

os.Getenv used to return two values, but we dropped that because we found that 99% of the time the second value was ignored. I don't think we should change back.

Perhaps we should add a function to os that returns two values. Or one that returns the environment in the form of a map. Or perhaps it's not worth worrying about.

@bradfitz
Copy link
Contributor

@bradfitz bradfitz commented Jan 23, 2015

I agree that we should not point people to the syscall package and if anything we could add a new function to the os package. I've written my own os.Getenv replacements a few times so I can determine between existence and blank.

@Stantheman
Copy link
Contributor Author

@Stantheman Stantheman commented Jan 23, 2015

I missed the lack of documentation in syscall -- my fault!

I didn't realize that Getenv used to return two values, thanks for clarifying @ianlancetaylor. I think either of your two first suggestions are nice. In my head os.Environ returns a map (similar to %ENV in perl and probably others), but that's in stone.

Perhaps? :

  1. os.GetenvCheck that returns syscall.Getenv()
  2. os.EnvironMap that takes syscall.Environ() and parses it first. Alternatively, it could be pushed down into the various env_* syscall levels to avoid extra work. I don't have a plan9 or Windows system to test these with though.

I'd be happy to take a stab at either/both solutions if they sound good.

@alexbrainman
Copy link
Member

@alexbrainman alexbrainman commented Feb 8, 2015

@Stantheman I will test your implementation on windows, if you like. FYI windows also distinguish between "this environment variable is blank" and "this environment variable does not exist".

Alex

@rsc
Copy link
Contributor

@rsc rsc commented Apr 10, 2015

@robpike for API design.
It should be in os not syscall.

@rsc rsc added this to the Go1.5Maybe milestone Apr 10, 2015
@robpike
Copy link
Contributor

@robpike robpike commented May 5, 2015

Suggest, for os package:

// LookupEnv returns the value of the environment variable with the given name.
// If there is no variable with that name, the boolean will be false.
func LookupEnv(name string) (string, bool)

@gopherbot
Copy link

@gopherbot gopherbot commented May 5, 2015

CL https://golang.org/cl/9741 mentions this issue.

@robpike robpike closed this in e8c0d0f May 5, 2015
@mikioh mikioh modified the milestones: Go1.5, Go1.5Maybe May 15, 2015
@golang golang locked and limited conversation to collaborators Jun 25, 2016
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Projects
None yet
Linked pull requests

Successfully merging a pull request may close this issue.

None yet
8 participants
You can’t perform that action at this time.