-
Notifications
You must be signed in to change notification settings - Fork 2
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
Do not pollute home dir #21
Conversation
This is ready to go but I made it draft until #20 is merged as this depends on it. |
ab99fd9
to
00270e5
Compare
No error from xdg.Home.
Otherwise use xdg which is smart about giving us the platform specific cache dir. See https://github.com/adrg/xdg#xdg-base-directory for values used.
00270e5
to
b11ac67
Compare
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
LGTM
cmd/root.go
Outdated
@@ -45,6 +45,6 @@ func Execute() { | |||
} | |||
|
|||
func init() { | |||
RootCmd.PersistentFlags().StringVar(&cfgFile, "config", "", "config file (default is $HOME/.mctl.yml)") | |||
RootCmd.PersistentFlags().StringVar(&cfgFile, "config", "", "config file (default is $XDG_CACHE_DIR/mctl/config.yml)") |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Shouldn't this be $XDG_CONFIG_HOME
instead? According to the spec https://specifications.freedesktop.org/basedir-spec/basedir-spec-latest.html, it's where the user config files should be located. This env var wouldn't be used on a Mac environment, AFAICT.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
1000% right... will fix
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
fixed, PTAL again thanks.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This env var wouldn't be used on a Mac environment, AFAICT.
The go package I'm using here basically does the right thing for other platforms too:
Unix-like operating systems
Unix | macOS | Plan 9 | |
---|---|---|---|
XDG_CONFIG_HOME | ~/.config | ~/Library/Application Support | $home/lib |
Microsoft Windows
Known Folder(s) | Fallback(s) | |
---|---|---|
XDG_CONFIG_HOME | LocalAppData | %LOCALAPPDATA% |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Not quite right still, diff shows $XDG_CONFIG_DIR
while it should be $XDG_CONFIG_HOME
.
Good to know it works everywhere! Thanks for the fixes!
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
thats what I get when I sed w/o thinking :/
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@diogomatsubara ptal again, thanks!
b11ac67
to
b4f254f
Compare
b4f254f
to
42a2ed2
Compare
🎉 |
uggh I very much dislike that merge commits aren't allowed in this org |
Lets be good cli folks and not pollute a user's $HOME, instead lets put config files in platform specified config paths. ~/.mctl.yml is used if it exists otherwise we look in dirs provided by xdg package.