-
Notifications
You must be signed in to change notification settings - Fork 85
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
deer
implement Deserialize
for built-in types (Part 1)
#1516
Conversation
deer
implement Deserialize
for built-in typesdeer
implement Deserialize
for built-in types
The latest updates on your projects. Learn more about Vercel for Git ↗︎
|
Codecov Report
@@ Coverage Diff @@
## main #1516 +/- ##
==========================================
+ Coverage 55.10% 57.05% +1.94%
==========================================
Files 251 260 +9
Lines 18076 18371 +295
Branches 421 421
==========================================
+ Hits 9961 10481 +520
+ Misses 8110 7885 -225
Partials 5 5
Flags with carried forward coverage won't be shown. Click here to find out more.
📣 We’re building smart automated test selection to slash your CI/CD build times. Learn more |
deer
implement Deserialize
for built-in typesdeer
implement Deserialize
for built-in types (Part 1)
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.
Looks great, Bilal! I left some comments. The error messages/structure and general composibility is turning out to look very neat!
@@ -1,3 +1,3 @@ | |||
[toolchain] | |||
channel = "nightly-2022-11-14" |
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.
Slightly unrelated to PR, but do we want to bump the channel soon?
T: Deserialize<'de> + Debug, | ||
Value: PartialEq<E>, |
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.
I haven't seen this sort of syntax before. Is Value
the serde_json
Value
Enum? Why do we need to set a specific bound for it to be PartialEq
here?
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.
Thanks! I need to change this, I initially added this for better diagnostics, but similar fails to display a diff between two different types properly. I need to change this so that the error is "casted" to the error enum instead. The bound is here because
assert_eq!
requires that the LHS implements PartialEq. Therefore the bound was needed, and not the other way around.
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.
reworked in af34989
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.
Looks great!
Co-authored-by: Ahmad Sattar <thehabbos007@gmail.com>
…er/stdlib # Conflicts: # packages/libs/deer/desert/src/token.rs
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.
Happy for this to merge. The remaining open comment is just a follow-up and does not blocking merging :)
🌟 What is the purpose of this PR?
This implements
Deserialize
for a select built-in types.🔗 Related links
🚫 Blocked by
📹 Demo