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

Add config_dir-based file option #400

Merged
merged 4 commits into from
May 10, 2023
Merged

Conversation

CosmicHorrorDev
Copy link
Contributor

Resolves #394

This uses the dirs crate to add <config_dir>/kanata/kanata.kbd to the config path

default_cfg() checks to see if the file exists before adding it to the path because the check later in cli_init() specifically checks the first config file. There is also a new error message for providing no config files provided since an empty vec of config files can be passed through now

@rszyma
Copy link
Contributor

rszyma commented May 8, 2023

Configuration file(s) to use with kanata. If not specified, defaults to
kanata.kbd in the current working directory and <config_dir>/kanata/kanata.kbd.

It might be a good idea to show actual path of the config dir rather than literally saying "<config_dir>" as it might be confusing to user where the <config_dir> is located.

@CosmicHorrorDev
Copy link
Contributor Author

That gets a bit messy with how clap's derive API is configured unfortunately. I'll look at how other programs document this to see if that gives me any better ideas

@jtroo
Copy link
Owner

jtroo commented May 8, 2023

Perhaps documenting it as ... $XDG_CONFIG_HOME/kanata/kanata.kbd. "$XDG_CONFIG_HOME" is usually "$HOME/.config". might be good enough?

@CosmicHorrorDev
Copy link
Contributor Author

That seems like a good middle ground to me

@rszyma
Copy link
Contributor

rszyma commented May 8, 2023

Isn't ~/.config/kanata/kanata.kbd always the same path as $XDG_CONFIG_HOME/kanata/kanata.kbd?

Another thing, dirs::config_dir also works on windows, so instead of this:

    /// kanata.kbd in the current working directory and $XDG_CONFIG_HOME/kanata/kanata.kbd.
    #[arg(short, long, verbatim_doc_comment)]
    cfg: Option<Vec<PathBuf>>,

We could do this:

    #[cfg(target_os = "windows")]
    /// Configuration file(s) to use with kanata. If not specified, defaults to
    /// kanata.kbd in the current working directory and C:\Users\<USER>\AppData\Roaming\kanata\kanata.kbd.
    #[arg(short, long, verbatim_doc_comment)]
    cfg: Option<Vec<PathBuf>>,

    #[cfg(not(target_os = "windows"))]
    /// Configuration file(s) to use with kanata. If not specified, defaults to
    /// kanata.kbd in the current working directory and ~/.config/kanata/kanata.kbd. 
    #[arg(short, long, verbatim_doc_comment)]
    cfg: Option<Vec<PathBuf>>,

@CosmicHorrorDev
Copy link
Contributor Author

Well it wouldn't be the same path if someone sets a different value for $XDG_CONFIG_HOME of course, but by default yes

You could also get cleaner cfgs, by doing something like

#[cfg_attr(target_os = "windows", doc = "I'm on windows")]
#[cfg_attr(not(target_os = "windows"), doc = "I'm not on windows")]

To avoid duplicating the struct's field

And then we'll also have to add one for Mac because they also use a different path too. I guess the paths are pretty stable, but not sure if it's worth the near copy-paste docs

@jtroo
Copy link
Owner

jtroo commented May 8, 2023

Fair points on the cross platform issues. Another idea is to revert to the original and link to https://docs.rs/dirs/latest/dirs/fn.config_dir.html for the reference of <config_dir>. Or just add all 3 OS' typical directories in one documentation item.

@CosmicHorrorDev
Copy link
Contributor Author

I opted for just displaying the platform specific paths of the big three (and assumed that other platforms would likely follow Linux's style). dirs's docs showed {FOLDERID_RoamingAppData} as the value for Windows, but I'm not familiar enough to know if that's an actual path or not, so I opted for just displaying an example path

I think this is a good balance of being direct while still showing the value for platforms other than Linux

@jtroo jtroo merged commit 067e565 into jtroo:main May 10, 2023
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.

Feature request: Allow for config file in typical system dependent locations
3 participants