-
Notifications
You must be signed in to change notification settings - Fork 317
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
Fix #861 serde::Deserialize impl for Url prints confusing error message #862
base: main
Are you sure you want to change the base?
Conversation
I think a better implementation would be E::invalid_value(Unexpected::Other(&err_s), &self) which then would keep the original "why it's broken" info intact:
or even simpler: Url::parse(s).map_err(E::custom)?; which still would be fine:
|
@denschub I respectfully disagree with your implementation as it does not pass the parameter intended in serde's API and causes a different problem. https://docs.rs/serde/latest/serde/de/trait.Error.html#method.invalid_value "The unexp argument provides information about what value was received. This is the value that was present in the input file or other source data of the Deserializer." "err_s" is not the invalid value, and the resulting message will not print the invalid value. In a real world you could have several fields of type Url, if you don't print the invalid value, you'd have no idea which of the fields is wrong. I understand that the reason it's invalid is relevant information, but it doesn't fit into serde's API. |
The documentation for
an invalid port number is "a thing", it was not expected, and the input contained it. Your PR changes the implementation in a way that's even less helpful. Seeing a
does not help anyone figure out why the parsing failed. "serde is meant to parse, not to validate" is a valid statement, but you need to parse before you can validate, and nobody is helped if parsing fails for reasons not clear to anyone, and for reasons that are not communicated. Also, what is being returned here is the result of Ultimately, though, I'm not a maintainer of this create. And I don't really mind either way, because I have ways to work around both solutions. |
Those are good points. Though in my opinion, if you want to provide a feature to provide an implementation for a particular trait it should do so in a way that's predictable and consistent with standard/core implementations for that trait. At the end of the day, I ignore the "serde" feature and just use the NewType pattern over Url to implement serde::Serialize and serde::Deserialize or whatever functionality I happen to need. After almost a year with no comment or response from anyone, I don't see that changing anytime soon. Thanks again for being the first to comment. |
Fixes #861