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

Improve error messaged by dropping untagged enums #586

Merged
merged 1 commit into from
Apr 2, 2023

Conversation

jonasbb
Copy link
Owner

@jonasbb jonasbb commented Apr 2, 2023

Untagged enums do not provide good error messages and likely never will,
given that there are multiple PRs which are just completely ignored
(serde#2376 and
serde#1544).

Instead using content::de the untagged enums can be replaced by custom
buffering. The error messages for OneOrMany and PickFirst now look
like this, including the original failure for each variant.

OneOrMany could not deserialize any variant:
  One: invalid type: map, expected u32
  Many: invalid type: map, expected a sequence
PickFirst could not deserialize any variant:
  First: invalid type: string "Abc", expected u32
  Second: invalid digit found in string

The implementations of VecSkipError and DefaultOnError are updated
too, but should not result in any visible changes.

Untagged enums do not provide good error messages and likely never will,
given that there are multiple PRs which are just completely ignored
([serde#2376](serde-rs/serde#2376) and
[serde#1544](serde-rs/serde#1544)).

Instead using `content::de` the untagged enums can be replaced by custom
buffering. The error messages for `OneOrMany` and `PickFirst` now look
like this, including the original failure for each variant.

```text
OneOrMany could not deserialize any variant:
  One: invalid type: map, expected u32
  Many: invalid type: map, expected a sequence
```

```text
PickFirst could not deserialize any variant:
  First: invalid type: string "Abc", expected u32
  Second: invalid digit found in string
```

The implementations of `VecSkipError` and `DefaultOnError` are updated
too, but should not result in any visible changes.
@jonasbb jonasbb force-pushed the drop-most-serde-derives branch from 6950301 to 73f9e40 Compare April 2, 2023 11:02
@codecov
Copy link

codecov bot commented Apr 2, 2023

Codecov Report

Merging #586 (73f9e40) into master (eb1965a) will increase coverage by 1.86%.
The diff coverage is 82.95%.

@@            Coverage Diff             @@
##           master     #586      +/-   ##
==========================================
+ Coverage   44.41%   46.27%   +1.86%     
==========================================
  Files          40       40              
  Lines        3249     3315      +66     
==========================================
+ Hits         1443     1534      +91     
+ Misses       1806     1781      -25     
Impacted Files Coverage Δ
serde_with/src/de/impls.rs 57.93% <82.95%> (+3.24%) ⬆️

... and 1 file with indirect coverage changes

📣 We’re building smart automated test selection to slash your CI/CD build times. Learn more

@jonasbb
Copy link
Owner Author

jonasbb commented Apr 2, 2023

bors r+

@bors
Copy link
Contributor

bors bot commented Apr 2, 2023

Build succeeded:

@bors bors bot merged commit 7d670c2 into master Apr 2, 2023
@bors bors bot deleted the drop-most-serde-derives branch April 2, 2023 11:35
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 this pull request may close these issues.

1 participant