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

Deeper integration with Serde #13

Closed
dtolnay opened this issue Feb 11, 2017 · 4 comments
Closed

Deeper integration with Serde #13

dtolnay opened this issue Feb 11, 2017 · 4 comments
Assignees
Milestone

Comments

@dtolnay
Copy link

dtolnay commented Feb 11, 2017

Loading configuration directly using Serde produces way better error messages than this crate. By converting everything to config::Value you lose all the valuable information about where everything came from.

As an example of good error messages, the config in Alacritty contains:

#[derive(Deserialize)]
struct Config {
    font: Font,

    /* other fields */
}

#[derive(Deserialize)]
struct Font {
    use_thin_strokes: bool,

    /* other fields */
}

They load it like this:

let config: Config = serde_yaml::from_str(&config_yml_path)?;

Suppose somebody typos a boolean:

font:
  use_thin_strokes: tru

The error would look like:

font.use_thin_strokes: invalid type: string "tru", expected a boolean at line 51 column 21

The error message gives the exact line and column to fix the error.

I think it should be possible to improve by implementing config::get as a DeserializeSeed that runs against the original config source (the yaml file or whatever). Basically the seed is the key that you are looking up and the DeserializeSeed implementation will deserialize that key from the config file, keeping track of whether that key was found. If not found, it can try again on the next source.

@mehcode
Copy link
Collaborator

mehcode commented Feb 11, 2017

I can see this when used in conjunction with a deserialzable.

Keep in mind that in the general case, I'd want this library to remain weakly typed. For instance, suppose that yaml was:

font:
  use_thin_strokes: 1

config::get_bool("font.use_thin_strokes") is going to return Some(true).

I don't mind having some way to change this to a more strict typing, as long as the API to do so feels fine.

@dtolnay
Copy link
Author

dtolnay commented Feb 11, 2017

The DeserializeSeed approach is 100% compatible with remaining weakly typed.

@mehcode
Copy link
Collaborator

mehcode commented Mar 3, 2017

Looking into this. I want to get this crate solid.

We can't easily run directly against the configuration source because of deep merging. A config::Deserializer could only work as a wrapper around config::Value.

I've been playing around with this and can now propagate simple type errors like ("tru" -> "true"). I'm planning to add locality to the error messages by tracking where each value came from in the main configuration hash by defining the hash as HashMap<String, (Value, Option<ValueSource>)>.

A ValueSource would be something like:

enum ValueSource {
  File { row: usize, column: usize },
  Uri(String)
}

I do have a couple questions for you, @dtolnay (and anyone else watching).

  1. How should the error message be formatted? Do these look good to you?
invalid type: string "tru", expected a boolean from config/default.toml at line 51, column 21
invalid type: string "fals", expected a boolean from etcd+http://127.0.0.1:2379
  1. Clearly a value coming from config.set_* does not have a source. Does a value coming from the environment have a source?
invalid type: string "fals", expected a boolean from the environment
invalid type: string "fals", expected a boolean from RUST_DEBUG

@mehcode mehcode self-assigned this Mar 3, 2017
@mehcode mehcode modified the milestone: 0.5.0 Mar 3, 2017
@mehcode
Copy link
Collaborator

mehcode commented Jun 14, 2017

This is all now on master

@mehcode mehcode closed this as completed Jun 14, 2017
epage added a commit to epage/config-rs that referenced this issue Oct 22, 2024
…on-3.x

chore(deps): update github/codeql-action action to v3
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

No branches or pull requests

2 participants