-
-
Notifications
You must be signed in to change notification settings - Fork 10
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
feat: read credentials.ini from XDG_CONFIG_HOME (or os equivalents) in addition to bin directory #103
base: main
Are you sure you want to change the base?
Conversation
… addition to bin directory
"linux people", as you put it, are definitely welcome here 😄 I'll try to give it a try today! Thanks for the PR! |
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.
Looking good! And the new dependency seems super useful too!
let xdg_config_path = dirs::config_local_dir().unwrap().join("discrakt"); | ||
let mut exe_path = env::current_exe().unwrap(); | ||
exe_path.pop(); | ||
|
||
let locations = vec![xdg_config_path, exe_path]; |
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.
let xdg_config_path = dirs::config_local_dir().unwrap().join("discrakt"); | |
let mut exe_path = env::current_exe().unwrap(); | |
exe_path.pop(); | |
let locations = vec![xdg_config_path, exe_path]; | |
let config_path = dirs::config_local_dir().unwrap().join("discrakt"); | |
let mut exe_path = env::current_exe().unwrap(); | |
exe_path.pop(); | |
let locations = vec![config_path, exe_path]; |
If dirs::config_local_dir()
works for all OSs, probably the best naming is simply config_path
?
|--------|-----|-------| | ||
|Linux|`$XDG_CONFIG_HOME`/discrakt or `$HOME`/.config/discrakt|/home/alice/.config/discrakt/credentials.ini| | ||
|macOS|`$HOME`/Library/Application Support/discrakt|/Users/Alice/Library/Application Support/discrakt/credentials.ini| | ||
|Windows|`%LOCALAPPDATA%`\discrakt|C:\Users\Alice\AppData\Local\discrakt\credentials.ini| |
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.
Isn't it this:
|Windows|`%LOCALAPPDATA%`\discrakt|C:\Users\Alice\AppData\Local\discrakt\credentials.ini| | |
|Windows|`%LOCALAPPDATA%`\discrakt|C:\Users\Alice\AppData\Roaming\discrakt\credentials.ini| |
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.
dirs::config_dir()
gives you Roaming, but dirs::config_local_dir()
gives you Local - not sure what the windows standard is to be honest - I just went with Local
%LOCALAPPDATA%
points to Local as well, think it's %APPDATA%
for Roaming
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.
Why not go with Roaming instead of Local?
Not only is it easier to type %AppData%
to navigate to it, but would also allow the config be backed up or migrated with the profile or even stay automatically synced across different devices in the same domain if the profile is attached to one.
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.
I'll be honest I didn't really know the difference between Local and Roaming! makes more sense to use Roaming then - cheers for mentioning
path.push("credentials.ini"); | ||
let config_file = find_config_file(); | ||
|
||
let path = config_file.expect("Could not find credentials.ini"); |
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.
I assume that the user needs to move the credentials.ini file inside a manually created distrakt
folder inside one of the config folders that are now documented in the readme?
Would be nice if it was created automatically on the first run before closing. This way the need to create/download the file manually would disappear! And we could probably open the file explorer in said file's location!
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.
so they're not required to move it into the new discrakt folder - if there is a config dir in the currently documented location it'll use that, but will also check the standard config dir
I was thinking a discrakt config
or discrakt init
flow might be a nice idea, but thought I'd keep it small and not make any big sweeping changes :D
Could absolutely put the config file in the default location and tell the user where it is and to modify it, maybe even have a flag to specify the location of the config file if they want to override for some reason
I'm just unsure how friendly the UX of telling someone that hadn't necessarily dug into the LOCALAPPDATA (or the macos Library location to go there and start tweaking files
could even maybe go through a setup flow on first launch if the file doesn't exist, to populate the file, put it in the location and then tell the user where it is - that might be the friendlier option
cheers - I've added a few replies, haven't had much time to look at it this week but will make some updates hopefully over the weekend happy to give the config population a go if you want unless you want to (no hard feelings either way) I feel like like I said in my replies - I'm cautious about making sweeping changes as a first time PR on the project :) |
📑 Description
Maybe it's just linux people but I'm not a fan of having configuration files in the same folder as the binary (as I installed in ~/.local/bin)
Each OS has its own 'assigned' configuration directory:
$XDG_CONFIG_HOME
or$HOME
/.config$HOME
/Library/Application Support{FOLDERID_LocalAppData}
Ideally the credentials.ini file would be managed by the cli tool and placed there, rather than the user doing it themselves, however I thought I'd keep the changes small.
I don't have a mac to test this on and haven't on windows yet (just linux)
✅ Checks
ℹ Additional Information
Note: I've only done a tiny bit of rust, apologies for any noobiness :)
Showing the error with multiple locations
Reading credentialas from bin directory (current behaviour)
Reading credentials from XDG_CONFIG_HOME and equivalents
It may be worth having additional support specifically for scoop now that I think of it, as that has a dedicated
persist
folder for config - something like%USERPROFILE%/scoop/persist/discrakt/credentials.ini
- I see credentials.ini is configured to persist, not sure how that works with the current relative directory stuff though