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: return an error from UserHomeDir to match UserCacheDir #28562

Closed
ianlancetaylor opened this issue Nov 2, 2018 · 7 comments
Closed

os: return an error from UserHomeDir to match UserCacheDir #28562

ianlancetaylor opened this issue Nov 2, 2018 · 7 comments
Labels
FrozenDueToAge NeedsFix The path to resolution is known, but the work has not been done. release-blocker
Milestone

Comments

@ianlancetaylor
Copy link
Contributor

os.UserHomeDir will be new in Go 1.12, added in https://golang.org/cl/139418 with the signature func UserHomeDir() string. The existing function UserCacheDir has the signature func UserCacheDir() (string, error). Should we change UserHomeDir to return an error as UserCacheDir does?

@ianlancetaylor ianlancetaylor added NeedsDecision Feedback is required from experts, contributors, and/or the community before a change can be made. release-blocker labels Nov 2, 2018
@ianlancetaylor ianlancetaylor added this to the Go1.12 milestone Nov 2, 2018
@andybons
Copy link
Member

andybons commented Nov 2, 2018

My vote is yes. Originally UserCacheDir returned a string but an empty string could lead to bugs that are hard to track down. I'm happy to send a CL if that's what we'd like to do.

@bradfitz
Copy link
Contributor

bradfitz commented Nov 2, 2018

I think an error makes its use kinda tedious.

I'd prefer to just return "/" if the env is unset and let things fail with a permission error later.

But I don't object if everybody else prefers an error.

@stapelberg
Copy link
Contributor

One benefit of not returning an error: that allows for flag definitions to easily default to a location relative to $HOME:

var configPath = flag.String("config",
  filepath.Join(os.UserHomeDir(), ".config", "i3"),
  "path to write configuration file to")

Of course, the above expression is not portable and should probably be replaced by os.UserConfigDir, which should respect $XDG_CONFIG_DIR on Linux? :)

Aside from that, I agree with @bradfitz in that $HOME is so rarely unset that not caring much about that case is an okay trade-off, and users who work in such strange environments will easily be able to make the connection between the error and the lack of a $HOME variable.

What about returning "$HOME is unset"? That way, the error message would be “plumbed through”.

@andybons
Copy link
Member

andybons commented Nov 2, 2018

What about returning "$HOME is unset"? That way, the error message would be “plumbed through”.

At that point we should just return an error in my opinion.

That said, I don't know enough about the circumstances of when $HOME would not be set (and whether this is a valid error condition). My vote was for API consistency going forward.

@jimmyfrasche
Copy link
Member

If it returns a descriptive error message that will probably make its way back to the user who can then say "Oh, this program isn't working because $HOME isn't set—I can investigate why that is happening and fix this".

Looking at the code, on windows, it's not as simple as returning the empty string on failure since it concatenates two environment variables so one could be set while the other not. It could check and return "" if either are "", without returning an error. But as the logic increases so does the usefulness of a clear error message explaining what went wrong where.

@bradfitz bradfitz added the NeedsFix The path to resolution is known, but the work has not been done. label Nov 14, 2018
@rsc
Copy link
Contributor

rsc commented Nov 14, 2018

Discussed with @bradfitz and @andybons. The missing error was mainly for convenience but hopefully the work on error handling will address that instead of losing the ability in the API to tell whether it succeeded.

@gopherbot gopherbot removed the NeedsDecision Feedback is required from experts, contributors, and/or the community before a change can be made. label Nov 14, 2018
@bradfitz bradfitz changed the title os: Should UserHomeDir match UserCacheDir? os: return an error from UserHomeDir to match UserCacheDir Nov 14, 2018
@gopherbot
Copy link

Change https://golang.org/cl/150418 mentions this issue: os: return an error from UserHomeDir to match UserCacheDir

@golang golang locked and limited conversation to collaborators Nov 22, 2019
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
FrozenDueToAge NeedsFix The path to resolution is known, but the work has not been done. release-blocker
Projects
None yet
Development

No branches or pull requests

7 participants