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

feat: read credentials.ini from XDG_CONFIG_HOME (or os equivalents) in addition to bin directory #103

Merged
merged 6 commits into from
Oct 9, 2024

Conversation

RobBrazier
Copy link
Contributor

📑 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:

Platform Value Example
Linux $XDG_CONFIG_HOME or $HOME/.config /home/alice/.config
macOS $HOME/Library/Application Support /Users/Alice/Library/Application Support
Windows {FOLDERID_LocalAppData} C:\Users\Alice\AppData\Local

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)

NOTE: This does introduce a small new dependency dirs, not sure how you'd feel about that - there's probably other ways to do it, but this seemed to be the 'easiest'

✅ Checks

  • code: My pull request adheres to the code style of this project
  • docs: Added / updated
  • tests: All the tests have passed

Note: all 0 tests passed via cargo test

ℹ Additional Information

Note: I've only done a tiny bit of rust, apologies for any noobiness :)

Showing the error with multiple locations

$ cargo run --release
   Compiling discrakt v2.2.3 (/home/rob/Code/github.com/RobBrazier/discrakt)
    Finished `release` profile [optimized] target(s) in 1.52s
     Running `target/release/discrakt`
Could not find credentials.ini in ["/home/rob/.config/discrakt", "/home/rob/Code/github.com/RobBrazier/discrakt/target/release"]
thread 'main' panicked at src/utils.rs:171:28:
Could not find credentials.ini
note: run with `RUST_BACKTRACE=1` environment variable to display a backtrace

Reading credentialas from bin directory (current behaviour)

$ ls target/release/credentials.ini
/home/rob/Code/github.com/RobBrazier/discrakt/target/release/credentials.ini
$ ls ~/.config/discrakt/credentials.ini
ls: cannot access '/home/rob/.config/discrakt/credentials.ini': No such file or directory
$ cargo run --release
    Finished `release` profile [optimized] target(s) in 0.02s
     Running `target/release/discrakt`
2024-08-11T16:14:49Z : One Piece - S4E109 - The Key to a Great Comeback Escape! The Wax-Wax Ball! | 7.64%

Reading credentials from XDG_CONFIG_HOME and equivalents

$ ls target/release/credentials.ini
ls: cannot access 'target/release/credentials.ini': No such file or directory
$ ls ~/.config/discrakt/credentials.ini
/home/rob/.config/discrakt/credentials.ini
$ cargo run --release
    Finished `release` profile [optimized] target(s) in 0.02s
     Running `target/release/discrakt`
2024-08-11T16:18:20Z : One Piece - S4E109 - The Key to a Great Comeback Escape! The Wax-Wax Ball! | 22.29%

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

@RobBrazier RobBrazier changed the title feat: read credentials.ini from XDG_CONFIG_DIR (or os equivalents) in addition to bin directory feat: read credentials.ini from XDG_CONFIG_HOME (or os equivalents) in addition to bin directory Aug 11, 2024
@afonsojramos
Copy link
Owner

afonsojramos commented Aug 12, 2024

"linux people", as you put it, are definitely welcome here 😄 I'll try to give it a try today! Thanks for the PR!

Copy link
Owner

@afonsojramos afonsojramos left a 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!

src/utils.rs Outdated Show resolved Hide resolved
README.md Outdated Show resolved Hide resolved
src/utils.rs Show resolved Hide resolved
@RobBrazier
Copy link
Contributor Author

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 clap may be beneficial to pull in, to allow for flags and/or subcommands to create/update the config, might make life easier, but should be possible without too

like I said in my replies - I'm cautious about making sweeping changes as a first time PR on the project :)

@afonsojramos
Copy link
Owner

Merging now, if we want some additional changes we can always ship them on a future release! Thank you for your help!

@afonsojramos afonsojramos merged commit e99ddfa into afonsojramos:main Oct 9, 2024
4 checks passed
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.

3 participants