Add XDG Support to confy
#119
Conversation
…configs to be stored in xdg defaults
deg4uss3r
force-pushed
the
rth/introduce_xdg
branch
from
0d982cc to
45bfa32
Compare
|
I'll take a look at this tomorrow! Thanks for the PR. |
There was a problem hiding this comment.
Additionally, I'm really uncomfortable with the idea that one would have a silent, seriously breaking change behind a flag that could get accidentally turned on by a dependency. It would be nice if we checked for both locations on macOS, and maybe had a way to select which method should be used by the downstream application (since, for example, GUI apps may want to put their non-user-editable configuration into ~/Library)
| @@ -11,12 +11,12 @@ edition = "2024" | |||
|
|
|||
| [dependencies] | |||
| ron = { version = "0.10.1", optional = true } | |||
| directories = "6" | |||
| etcetera = "0.10.0" | |||
There was a problem hiding this comment.
Should we add a link to https://docs.rs/etcetera/latest/etcetera/#native-strategy (or the discussion we had here) to let peoples know how to configure confy? At least some documentation (or example/test) around it to show can be useful.
| @@ -1,6 +1,6 @@ | |||
| [package] | |||
| name = "confy" | |||
| version = "1.0.0" | |||
| version = "2.0.0" | |||
There was a problem hiding this comment.
I’m not sure we have a change-log file, but writing somewhere what made change the major version can help peoples migrate.
There was a problem hiding this comment.
100% once we work out the API I am going to add into here: https://github.com/rust-cli/confy/?tab=readme-ov-file#breaking-changes
| fn get_configuration_directory_str(project: &impl AppStrategy) -> Result<String, ConfyError> { | ||
| Ok(project.config_dir().display().to_string()) |
There was a problem hiding this comment.
| fn get_configuration_directory_str(project: &impl AppStrategy) -> Result<String, ConfyError> { | |
| Ok(project.config_dir().display().to_string()) | |
| fn get_configuration_directory_str(project: &impl AppStrategy) -> Result<PathBuf, ConfyError> { | |
| Ok(project.config_dir()) |
I know it is a private function, but maybe it is better to use the PathBuf directly instead of relying on the String approximation?
Especially since you want to do a major version.
There was a problem hiding this comment.
Also, I think the function is used only once… it was a nice helper but may be useless now?
| @@ -469,23 +469,26 @@ pub fn get_configuration_file_path<'a>( | |||
| config_name: impl Into<Option<&'a str>>, | |||
| ) -> Result<PathBuf, ConfyError> { | |||
| let config_name = config_name.into().unwrap_or("default-config"); | |||
| let project = ProjectDirs::from("rs", "", app_name).ok_or_else(|| { | |||
| ConfyError::BadConfigDirectory("could not determine home directory path".to_string()) | |||
| let project = choose_app_strategy(AppStrategyArgs { | |||
There was a problem hiding this comment.
Oh I just understand now that you are "forcing" the app strategy? Not configurable then? (Still needs to document this.)
There was a problem hiding this comment.
Actually I think you're right we should allow this to be configurable for GUI applications, I'll update!
This PR:
directoriescrate in favor ofetcetera2.0.0$XDG_CONFIG_HOMEviaetceteraAppStrategyrulesrustfmtfrom version1.88Resolves #117