duplicate derives on ArchivedT types (for rkyv feature) #1271
Conversation
Codecov Report
@@ Coverage Diff @@
## 0.4.x #1271 +/- ##
==========================================
- Coverage 91.40% 91.28% -0.13%
==========================================
Files 38 38
Lines 16932 16981 +49
==========================================
+ Hits 15477 15501 +24
- Misses 1455 1480 +25
... and 10 files with indirect coverage changes 📣 We’re building smart automated test selection to slash your CI/CD build times. Learn more |
|
Thank you for working on this. I still haven't worked with rkyv and can't say anything meaningful yet. |
|
We can ignore coverage for this PR in my opinion. |
|
Other types could probably use it too, but personally I only need it for |
Awpteamoose
changed the title
I have added similar derives for other types that use rkyv and changed the PR title. |
| @@ -192,6 +192,10 @@ impl Days { | |||
| /// [proleptic Gregorian date]: crate::NaiveDate#calendar-date | |||
| #[derive(PartialEq, Eq, Hash, PartialOrd, Ord, Copy, Clone)] | |||
| #[cfg_attr(feature = "rkyv", derive(Archive, Deserialize, Serialize))] | |||
| #[cfg_attr( | |||
| feature = "rkyv", | |||
| archive_attr(derive(Clone, Copy, PartialEq, Eq, PartialOrd, Ord, Debug, Hash)) | |||
There was a problem hiding this comment.
Deriving those traits for the archived type pretty much makes us promise that NaiveDate will not be modified in a way that makes comparing the type any different from a plain integer. But I think that is a useful property anyway.
There was a problem hiding this comment.
If DateImpl changes you'll have to implement Eq/Ord in a way consistent with the current implementation, in which case it should be possible to implement rkyv traits and Eq/Ord on the Archived type in the same way. What I mean is, unless I'm misunderstanding, deriving traits for the Archived type doesn't make you promise anything you don't promise already.
There was a problem hiding this comment.
True. But that is not all that easy, and would amount to deserializing the Archived type. That seems to defeat the purpose.
Anyway I think we are good.
|
Unrelated to this PR: the cc @mkatychev. |
|
Opened a PR to add an |
Sounds good, I'll resolve the merge conflicts and add the tests there |
|
@djc Shall we merge this PR first? |
WIthout the derives ArchivedDuration loses a lot of its usefulness.