-
Notifications
You must be signed in to change notification settings - Fork 6
fix: fix incorrect credentials path, create common config logic #231
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
base: main
Are you sure you want to change the base?
Conversation
| userdir, err := os.UserConfigDir() | ||
| if err != nil { | ||
| return "", fmt.Errorf("failed to get dbc configuration directory: %v", err) | ||
| } | ||
|
|
||
| orgDirName := "columnar" | ||
| if runtime.GOOS == "windows" || runtime.GOOS == "darwin" { | ||
| orgDirName = "Columnar" | ||
| } | ||
| dbcDirName := "dbc" |
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.
but you were the one who said I should use XDG_DATA_HOME if it exists.... lol
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.
Oh right. 🤦 This still fixes the original issue that was reported. Let me think about it and I'll tweak this so it uses XDG_DATA_DIR on Linuxes.
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.
Which would make this:
- macOS + Windows: os.UserConfigDir()
- Linux: check XDG_DATA_DIR, fallback to os.UserConfigDir()
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.
it should be XDG_DATA_HOME, there is no XDG_DATA_DIR there is XDG_DATA_DIRS which is a list of paths
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.
and we should still check XDG_DATA_HOME on macOS too. According to Apple's docs, the equivalent for XDG_DATA_HOME on macOS is ~/Library/
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.
Oh, that last bit I didn't know.
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.
Maybe my idea was bad, I'll think about it more when I'm back.
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.
yea, that's why it was using the logic I laid out. which was separate from the XDG_CONFIG_HOME (os.UserConfigDir) heh
|
Did we come to a consensus on this? |
Closes #230