jonasbb / serde_with

This crate provides custom de/serialization helpers to use in combination with serde's `with`-annotation and with the improved `serde_as`-annotation.
https://docs.rs/serde_with
Apache License 2.0
636 stars 67 forks source link

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

Closed johnmave126 closed 2 months ago

johnmave126 commented 2 months ago

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

Unanswered Questions

codecov[bot] commented 2 months ago

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 :warning:
Additional details and impacted files ```diff @@ 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 ```

:umbrella: View full report in Codecov by Sentry.
:loudspeaker: Have feedback on the report? Share it here.

jonasbb commented 2 months ago

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 commented 2 months ago

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 commented 2 months ago

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