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

Use XDG Base Directory Spec for default config file. #1061

Closed
wants to merge 3 commits into from

Conversation

pabryan
Copy link

@pabryan pabryan commented Dec 27, 2015

Fixes #1048. Note that the dependency cep21/xdgbasedir referenced in Godeps breaks the tests, though the committed version in _workspaces passes. Once cep21/xdgbasedir#1 is resolved upstream, then everything will be fine.

@pabryan
Copy link
Author

pabryan commented Dec 28, 2015

I don't know why the Travis build failed. The Travis build https://travis-ci.org/pabryan/hub/jobs/99089471 worked fine after committing to my fork pabryan/hub/@4c612cfa4

@pabryan
Copy link
Author

pabryan commented Jan 1, 2016

I've added a write test now. It's a little bit of a kludge since it uses authenticates first because as far as I know, the only place hub writes to the config file is when authenticating: if the authentication code breaks this test will fail even if the xdgbasedir code is working. I suppose I should also add a read config test too.

@@ -12,6 +12,7 @@ import (
"github.com/github/hub/Godeps/_workspace/src/github.com/howeyc/gopass"
"github.com/github/hub/ui"
"github.com/github/hub/utils"
"github.com/github/hub/Godeps/_workspace/src/github.com/cep21/xdgbasedir"
Copy link
Owner

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Instead of pulling in a new dependency, couldn't we simply read the value of XDG_CONFIG_HOME environment variable and default to $HOME/.config if not set?

Copy link
Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Sure. I'll do it later today if that's preferred. I just figured that it might be nice to have the xdgbasedir spec available in it's entirety, and why not pull it in from a library?. For instance there is an argument that passwords should go in data not in config, and the dependency has code for data directories as well. Hub config would go in the config directory. If a user wants to cache their password, but not save it permanently that could go in the cache directory etc.

Copy link
Owner

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

My general rule for software is not to pull in dependencies when you can simply inline something. This is the entirety of the logic that we're obtaining by pulling in an external library:

// DataHomeDirectory is a single base directory relative to which user-specific data files should be written
func DataHomeDirectory() (string, error) {
    return getInEnvOrJoinWithHome("XDG_DATA_HOME", defaultDataHomeDirectory)
}

we can do the same ourselves. Also, this library uses user.Current(), which we want to avoid due to cross-compilation issues: #1082

@mislav
Copy link
Owner

mislav commented Jan 27, 2016

Due to some changes in master, this PR will now heavily conflict at merge. @pabryan do you want me to take this over, since you're busy at the moment? I can sort it out quickly.

@pabryan
Copy link
Author

pabryan commented Jan 27, 2016

Okay, thanks.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

respect the XDG_CONFIG_HOME environment variable
2 participants