-
Notifications
You must be signed in to change notification settings - Fork 183
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
Add RcBuf variant to DataPayload #816
Conversation
I added a new variant to DataPayload for buffers and wired it up in FsDataProvider and StaticDataProvider. I used the following bound on functions where the data struct needs to be deserializable: for<'de> <M::Yokeable as Yokeable<'de>>::Output: serde::de::Deserialize<'de>, Good news:
Bad news:
I believe this is rust-lang/rust#85636 again. Unfortunately, I don't think I can use the "reference workaround" here again, because I care about @Manishearth Thoughts? UPDATE: Manish answered my question and posted the solution in #85636. |
Codecov Report
@@ Coverage Diff @@
## main #816 +/- ##
==========================================
+ Coverage 75.32% 75.43% +0.11%
==========================================
Files 192 193 +1
Lines 12374 12499 +125
==========================================
+ Hits 9321 9429 +108
- Misses 3053 3070 +17
Continue to review full report at Codecov.
|
Pull Request Test Coverage Report for Build f0b79ddad05faf2b5d1be5e40ef90cd90c1e61e4-PR-816
💛 - Coveralls |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Overall looks pretty good! Glad to see this coming together so nicely.
provider/core/src/serde.rs
Outdated
T: serde::Deserialize<'de>, | ||
M: DataMarker<'s>, | ||
M::Yokeable: serde::de::Deserialize<'static>, | ||
for<'de> SerdeDeDataStructWrap<<M::Yokeable as Yokeable<'de>>::Output>: |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
suggestion: Please add a comment with the actual bound we are trying to express here (and a link to the issue)
(ditto everywhere else the hack wrap is used)
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
There are 4 places and I made them all look like this:
// Actual bound:
// for<'de> <M::Yokeable as Yokeable<'de>>::Output: serde::de::Deserialize<'de>,
// Necessary workaround bound (see yoke docs):
for<'de> YokeTraitHack<<M::Yokeable as Yokeable<'de>>::Output>: serde::de::Deserialize<'de>,
I added |
Docs preview on |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
small nit, r=me otherwise
See #667