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

Implement MapSkipError, analogous to VecSkipError, but for map-like data structures. #763

Merged
merged 4 commits into from
Jul 12, 2024

Conversation

johnmave126
Copy link
Contributor

@johnmave126 johnmave126 commented Jul 3, 2024

Currently, there is a VecSkipError for resiliently handling heterogenous sequences. However, there are also heterogenous maps in the wild, notably the bundle version information plist from Apple.

This PR implements MapSkipError, analogous to VecSkipError, deserializes an entry only if both the key and value are deserializable.

Notable Changes

  • The impl of VecSkipError is moved to a new file de/skip_error.rs and ser/skip_error.rs to reuse GoodOrError with MapSkipError.
  • The impl of MapSkipError works for all the maps enumerated by impls::macros::foreach_map.

Unanswered Questions

  • Do we also need to implement this for kv-tuple lists?
  • Do we also need to implement modules under serde_with::rust for BTreeMap and HashMap?

Copy link

codecov bot commented Jul 3, 2024

Codecov Report

Attention: Patch coverage is 79.41176% with 7 lines in your changes missing coverage. Please review.

Project coverage is 67.32%. Comparing base (dee706a) to head (94ff4c1).
Report is 7 commits behind head on master.

Files Patch % Lines
serde_with/src/de/skip_error.rs 76.66% 7 Missing ⚠️
Additional details and impacted files
@@            Coverage Diff             @@
##           master     #763      +/-   ##
==========================================
+ Coverage   67.25%   67.32%   +0.07%     
==========================================
  Files          38       40       +2     
  Lines        2452     2464      +12     
==========================================
+ Hits         1649     1659      +10     
- Misses        803      805       +2     

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.

@jonasbb
Copy link
Owner

jonasbb commented Jul 12, 2024

Thanks for the PR, this looks all really good.

Regarding your questions:

  1. It is not necessary to support kv-list. You can add support if you want, but this PR is fine to merge as is.
  2. No, you shouldn't add more to serde_with::rust. The VecSkipError doesn't have any other variations either. In general, it is difficult to combine such different behaviors and if you combine too many you have a giant compatibility matrix.

@johnmave126
Copy link
Contributor Author

Cooool.

You are right. Under the current way of enumerating different types of map, adding support to kv-list probably means adding support for MapPreventDuplicates and alike, which is out of the scope of this PR. I don't plan to further change this PR then.

@jonasbb jonasbb merged commit d038657 into jonasbb:master Jul 12, 2024
18 checks passed
@jonasbb
Copy link
Owner

jonasbb commented Jul 12, 2024

Thanks for the work :) I try to prepare a new release this weekend.

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.

None yet

2 participants