Improve error messages when deserialising enums - #1376
Conversation
I believe it was only used by the `serde_string_enum` crate (which for some reason did not hide this behind a feature flag).
Codecov Report✅ All modified and coverable lines are covered by tests. Additional details and impacted files@@ Coverage Diff @@
## main #1376 +/- ##
=======================================
Coverage 89.68% 89.68%
=======================================
Files 58 58
Lines 8406 8406
Branches 8406 8406
=======================================
Hits 7539 7539
Misses 550 550
Partials 317 317 ☔ View full report in Codecov by Harness. 🚀 New features to boost your workflow:
|
There was a problem hiding this comment.
Pull request overview
This PR refactors several user-facing string enums to use vanilla serde (#[derive(Deserialize)] + #[serde(rename = "...")]) instead of serde_string_enum, improving deserialisation error messages (e.g., listing expected variants) and dropping the serde_string_enum/unicase dependencies.
Changes:
- Replace
DeserializeLabeledStringEnumusage withserde-native enum deserialisation across multiple input enums. - Remove
serde_string_enumandunicasefrom the dependency tree (and updateCargo.lockaccordingly). - Align enum variant string mappings via
#[serde(rename = "...")]for clearer parse failures.
Reviewed changes
Copilot reviewed 5 out of 6 changed files in this pull request and generated 2 comments.
Show a summary per file
| File | Description |
|---|---|
| src/time_slice.rs | Switch TimeSliceLevel to serde renames; remove serde_string_enum import. |
| src/process.rs | Switch FlowType to serde renames and native Deserialize. |
| src/commodity.rs | Switch commodity-related enums to serde renames and native Deserialize. |
| src/agent.rs | Switch ObjectiveType to serde renames and native Deserialize. |
| Cargo.toml | Drop serde_string_enum and unicase dependencies. |
| Cargo.lock | Remove serde_string_enum and unicase packages from the lockfile. |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
| use itertools::Itertools; | ||
| use serde::de::Error; | ||
| use serde::{Deserialize, Serialize}; |
There was a problem hiding this comment.
Yeah... this breaks compilation 🤷
tsmbland
left a comment
There was a problem hiding this comment.
Looks good. Can we now make the general claim that all fields are case sensitive, or is that not the case?
dc2917
left a comment
There was a problem hiding this comment.
Looks good to me - simple change to the code that does indeed provide a more informative error message. Having the case-sensitivity documented is good!
AdrianDAlessandro
left a comment
There was a problem hiding this comment.
Removing unnecessary requirements and getting better error messages in one fell swoop? Nice!
Description
The error messages you get when deserialising an enum fails are currently not very informative, e.g.:
We use the
serde_string_enumcrate for performing this deserialisation, but it turns out you can just use vanillaserdeto do basically the same thing, by annotating enum variants with#[serde(rename = ..., e.g.:And you get much nicer error messages!
There is one downside to this alternative approach, namely that it doesn't match the string in a case insensitve manner, so where
LCOXwould have matched before, now onlylcoxwill match. If this does happen though, at least the user will get an informative message about it 😄. You can add other variants that are accepted, so we could accept bothlcoxandLCOX(Lcoxwould still be rejected), but I'm not sure it's worth it.I first introduced the
serde_string_enumdependency in #105 and didn't mention the case-insensitive thing there, so it presumably wasn't a big consideration (incidentally, it hasn't been updated since).I think the trade-off is worth it for better error messages. We can also drop two dependencies this way, which is also nice.
Closes #848.
Type of change
Key checklist
$ cargo test$ cargo docpresent in the previous release
Further checks