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

Comments

@ianlancetaylor
Copy link
Contributor

@ianlancetaylor ianlancetaylor commented Nov 2, 2018

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?

@andybons

This comment has been minimized.

Copy link
Member

@andybons 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

This comment has been minimized.

Copy link
Contributor

@bradfitz 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

This comment has been minimized.

Copy link
Contributor

@stapelberg stapelberg commented Nov 2, 2018

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

This comment has been minimized.

Copy link
Member

@andybons 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

This comment has been minimized.

Copy link
Member

@jimmyfrasche jimmyfrasche commented Nov 2, 2018

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.

@rsc

This comment has been minimized.

Copy link
Contributor

@rsc 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.

@bradfitz bradfitz changed the title os: Should UserHomeDir match UserCacheDir? os: return an error from UserHomeDir to match UserCacheDir Nov 14, 2018
@gopherbot

This comment has been minimized.

Copy link

@gopherbot gopherbot commented Nov 20, 2018

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

@gopherbot gopherbot closed this in 649b893 Nov 22, 2018
@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.
Projects
None yet
Linked pull requests

Successfully merging a pull request may close this issue.

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