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

Regression in quality of TOML type mismatch error messages #3790

Closed
crumblingstatue opened this issue Mar 2, 2017 · 4 comments · Fixed by #3794
Closed

Regression in quality of TOML type mismatch error messages #3790

crumblingstatue opened this issue Mar 2, 2017 · 4 comments · Fixed by #3794

Comments

@crumblingstatue
Copy link

$ cargo +stable build
...
  expected a value of type `string`, but found a value of type `integer` for the key `dependencies.regex`

$ cargo build
...
  data did not match any variant of untagged enum TomlDependency for key `dependencies.regex`

Fixing this might require changes in toml, or maybe even serde, but I don't think we should sacrifice user experience for the developer productivity that serde provides.

@alexcrichton
Copy link
Member

Thanks for the report!

@dtolnay do you know if there's an easy-ish way to configure that error message locally?

@dtolnay
Copy link
Member

dtolnay commented Mar 2, 2017

Yes the untagged enum error messages need some work. We are tracking this in serde-rs/serde#773.

For now you can provide a custom message by writing the Deserialize impl yourself.

invalid type: integer `1`, expected a version string like "0.9.8" or a detailed dependency like {version = "0.9.8"} for key `dependencies.regex`

use serde::de::{self, Deserialize, Deserializer, Visitor};

impl Deserialize for TomlDependency {
    fn deserialize<D>(deserializer: D) -> Result<Self, D::Error>
        where D: Deserializer
    {
        struct TomlDependencyVisitor;

        impl Visitor for TomlDependencyVisitor {
            type Value = TomlDependency;

            fn expecting(&self, formatter: &mut fmt::Formatter) -> fmt::Result {
                formatter.write_str("a version string like \"0.9.8\" or a \
                                     detailed dependency like {version = \"0.9.8\"}")
            }

            fn visit_str<E>(self, s: &str) -> Result<Self::Value, E>
                where E: de::Error
            {
                Ok(TomlDependency::Simple(s.to_owned()))
            }

            fn visit_map<V>(self, map: V) -> Result<Self::Value, V::Error>
                where V: de::MapVisitor
            {
                let mvd = de::value::MapVisitorDeserializer::new(map);
                DetailedTomlDependency::deserialize(mvd).map(TomlDependency::Detailed)
            }
        }

        deserializer.deserialize(TomlDependencyVisitor)
    }
}

@crumblingstatue
Copy link
Author

Might be worth the extra effort to implement the above, since that error message is even better than the original.

@alexcrichton
Copy link
Member

@dtolnay awesome, thanks! @crumblingstatue yeah I'll send a PR.

alexcrichton added a commit to alexcrichton/cargo that referenced this issue Mar 8, 2017
Unfortunately while `#[serde(untagged)]` is precisely what we want in terms of
semantics it leaves a little to be desired in terms of error messages. This
commit updates to remove the usage of that attribute in favor of implementing
`Deserialize` directly, which is quite simple in these few cases.

Closes rust-lang#3790
bors added a commit that referenced this issue Mar 8, 2017
Improve TOML decoding error messages

Unfortunately while `#[serde(untagged)]` is precisely what we want in terms of
semantics it leaves a little to be desired in terms of error messages. This
commit updates to remove the usage of that attribute in favor of implementing
`Deserialize` directly, which is quite simple in these few cases.

Closes #3790
@bors bors closed this as completed in #3794 Mar 8, 2017
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 a pull request may close this issue.

3 participants