Skip to content

Add XDG Support to confy #119

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.

Sign up for GitHub

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

Open
wants to merge 1 commit into
base: master
Choose a base branch
from
Open

Add XDG Support to confy #119

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
1 commit
Select commit
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
3 changes: 2 additions & 1 deletion Cargo.toml
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -11,7 +11,7 @@ edition = "2024"

[dependencies]
ron = { version = "0.10.1", optional = true }
directories = "6"
etcetera = "0.10.0"
serde = "^1.0"
serde_yaml = { version = "0.9", optional = true }
thiserror = "2.0"
Expand All @@ -24,6 +24,7 @@ toml_conf = ["toml"]
basic_toml_conf = ["basic-toml"]
yaml_conf = ["serde_yaml"]
ron_conf = ["ron"]
xdg = []

[[example]]
name = "simple"
Expand Down
5 changes: 2 additions & 3 deletions examples/simple.rs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -41,7 +41,7 @@ fn main() -> Result<(), confy::ConfyError> {
name: "Test".to_string(),
..cfg
};
confy::store("confy_simple_app",None, &cfg)?;
confy::store("confy_simple_app", None, &cfg)?;
println!("The updated toml file content is:");
let mut content = String::new();
std::fs::File::open(&file)
Expand All @@ -53,7 +53,6 @@ fn main() -> Result<(), confy::ConfyError> {
name: "Test".to_string(),
..cfg
};
std::fs::remove_dir_all(file.parent().unwrap())
.expect("Failed to remove directory");
std::fs::remove_dir_all(file.parent().unwrap()).expect("Failed to remove directory");
Ok(())
}
68 changes: 50 additions & 18 deletions src/lib.rs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -73,9 +73,9 @@
//!
//! ### Tip
//! to add this crate to your project with the default, toml config do the following: `cargo add confy`, otherwise do something like: `cargo add confy --no-default-features --features yaml_conf`, for more info, see [cargo docs on features]
//!
//!
//! [cargo docs on features]: https://docs.rust-lang.org/cargo/reference/resolver.html#features
//!
//!
//! feature | file format | description
//! ------- | ----------- | -----------
//! **default**: `toml_conf` | [toml] | considered a reasonable default, uses the standard-compliant [`toml` crate]
Expand All @@ -94,8 +94,12 @@
mod utils;
use utils::*;

use directories::ProjectDirs;
use serde::{de::DeserializeOwned, Serialize};
#[cfg(feature = "xdg")]
use etcetera::app_strategy::choose_app_strategy;
#[cfg(not(feature = "xdg"))]
use etcetera::app_strategy::choose_native_strategy;
use etcetera::{AppStrategy, AppStrategyArgs};
use serde::{Serialize, de::DeserializeOwned};
use std::fs::{self, File, OpenOptions, Permissions};
use std::io::{ErrorKind::NotFound, Write};
use std::path::{Path, PathBuf};
Expand All @@ -109,8 +113,8 @@ use toml::{

#[cfg(feature = "basic_toml_conf")]
use basic_toml::{
from_str as toml_from_str, to_string as toml_to_string_pretty, Error as TomlDeErr,
Error as TomlSerErr,
Error as TomlDeErr, Error as TomlSerErr, from_str as toml_from_str,
to_string as toml_to_string_pretty,
};

#[cfg(not(any(
Expand Down Expand Up @@ -469,23 +473,49 @@ 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;

#[cfg(not(feature = "xdg"))]
{
Copy link

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

It would be nice to have a path for people to upgrade over time; this seems to preclude anyone ever switching, since it doesn't check for both.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I agree with your points above, I think what I am struggling with a little is how much this library should do versus how much the caller should do.

I do think it is unlikely that the flag would be turned on without the application author knowing. This also isn't destructive, and a config file can be moved by either the application and/or user, so I am bit torn over what the best way to do a migration is.

Maybe that is just making a new function that uses the xdg path, allowing it to still be opt in but less surprising if the feature is enabled.

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

To me this crate should not do to much. When I found it, it was just a wrapper over dir/directories/etcetera (choosing the correct one for me) and a choice of data format adapted for configuration.

At firsts it did not even give the config file’s path to the dev or user. I added this function so it is more easy to migrate away from confy, or just let the user know where the file is so they can edit the file with their editor.

I did not use this crate from ages, but if my mind it was the "easy little default crate", and if I wanted a bit more functionality I either build on top, or do manually/find another crate. Like, if:

  • I want 2 config file
  • I want to change my app name so the config file’s location
  • I want to change the data format

In all this cases, it is on me the application dev to find works around. I loved that this crates kept it simple.

Now it may very well evolve to be much more… but I think a new dev should be able to quickly choose between confy and config-rs. (Don’t forget: both are under the rust-cli organization.)

Copy link

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@strega-nil I agree with your point for the migration and support, and I agree that with @Zykino to keep it simple.

Thus I suggest to improve the code as following:

  • Make xdg feature default.
  • Add xdg_migration feature enabled by default.
  • If xdg feature disabled — use native folder (no xdg)
  • If xdg enabled and xdg_migration disabled — use xdg folder.
  • If xdg and xdg_migration features included:
    • check native folder. If exists — proceed with it
    • use xdg folder as defined in etcetera.
  • cache the result.

What do you all think on this?

project = choose_native_strategy(AppStrategyArgs {
top_level_domain: "rs".to_string(),
author: "".to_string(),
app_name: app_name.to_string(),
})
.map_err(|e| {
ConfyError::BadConfigDirectory(format!("could not determine home directory path: {e}"))
})?;
}

#[cfg(feature = "xdg")]
{
project = choose_app_strategy(AppStrategyArgs {
top_level_domain: "rs".to_string(),
author: "".to_string(),
app_name: app_name.to_string(),
})
.map_err(|e| {
ConfyError::BadConfigDirectory(format!("could not determine home directory path: {e}"))
})?;
}

let config_dir_str = get_configuration_directory_str(&project)?;

let path = [config_dir_str, &format!("{config_name}.{EXTENSION}")]
let path = [config_dir_str, format!("{config_name}.{EXTENSION}")]
.iter()
.collect();

Ok(path)
}

fn get_configuration_directory_str(project: &ProjectDirs) -> Result<&str, ConfyError> {
let path = project.config_dir();
path.to_str()
.ok_or_else(|| ConfyError::BadConfigDirectory(format!("{path:?} is not valid Unicode")))
fn get_configuration_directory_str(project: &impl AppStrategy) -> Result<String, ConfyError> {
let path = if cfg!(feature = "xdg") {
project.config_dir()
} else {
project.data_dir()
};

Ok(format!("{}", path.display()))
}

#[cfg(test)]
Expand Down Expand Up @@ -601,10 +631,12 @@ mod tests {

store_path_perms(path, &config, permissions).expect("store_path_perms failed");

assert!(fs::metadata(path)
.expect("reading metadata failed")
.permissions()
.readonly());
assert!(
fs::metadata(path)
.expect("reading metadata failed")
.permissions()
.readonly()
);
})
}

Expand Down
Loading