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

duplicate derives on ArchivedT types (for rkyv feature) #1271

Merged
merged 3 commits into from
Sep 22, 2023

Conversation

Awpteamoose
Copy link
Contributor

WIthout the derives ArchivedDuration loses a lot of its usefulness.

@codecov
Copy link

codecov bot commented Sep 8, 2023

Codecov Report

Merging #1271 (44fe133) into 0.4.x (77d0bec) will decrease coverage by 0.13%.
Report is 19 commits behind head on 0.4.x.
The diff coverage is 25.00%.

@@            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     
Files Changed Coverage Δ
src/duration.rs 91.86% <0.00%> (-0.20%) ⬇️
src/month.rs 75.00% <0.00%> (-0.30%) ⬇️
src/naive/date.rs 96.25% <0.00%> (-0.10%) ⬇️
src/naive/datetime/mod.rs 97.25% <0.00%> (-0.46%) ⬇️
src/naive/isoweek.rs 98.16% <0.00%> (-0.91%) ⬇️
src/offset/fixed.rs 79.45% <0.00%> (-0.55%) ⬇️
src/offset/local/mod.rs 85.32% <0.00%> (-2.65%) ⬇️
src/offset/utc.rs 84.21% <0.00%> (-1.51%) ⬇️
src/weekday.rs 82.38% <0.00%> (-0.40%) ⬇️
src/naive/time/mod.rs 95.83% <100.00%> (-0.68%) ⬇️

... and 10 files with indirect coverage changes

📣 We’re building smart automated test selection to slash your CI/CD build times. Learn more

@pitdicker
Copy link
Collaborator

Thank you for working on this. I still haven't worked with rkyv and can't say anything meaningful yet.
Wat is the reason only Duration needs this and not the other types?

@pitdicker
Copy link
Collaborator

We can ignore coverage for this PR in my opinion.
But do we have any tests at all for the rkyv feature?

@Awpteamoose
Copy link
Contributor Author

Other types could probably use it too, but personally I only need it for Duration. I could probably go around and see where it makes sense to add.

@Awpteamoose Awpteamoose changed the title duplicate derives on ArchivedDuration (for rkyv feature) duplicate derives on ArchivedT types (for rkyv feature) Sep 13, 2023
@Awpteamoose
Copy link
Contributor Author

Thank you for working on this. I still haven't worked with rkyv and can't say anything meaningful yet. Wat is the reason only Duration needs this and not the other types?

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))
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

@pitdicker
Copy link
Collaborator

Unrelated to this PR: the rkyv derive creates types such as ArchivedDuration in private modules. Maybe we should have a top-level rkyv module that re-exports them?

cc @mkatychev.

@pitdicker
Copy link
Collaborator

Opened a PR to add an rkyv module #1304.

@mkatychev
Copy link
Contributor

Opened a PR to add an rkyv module #1304.

Sounds good, I'll resolve the merge conflicts and add the tests there

@pitdicker
Copy link
Collaborator

@djc Shall we merge this PR first?

Copy link
Member

@djc djc left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Seems okay, please squash during merge.

@pitdicker pitdicker merged commit a32d77a into chronotope:0.4.x Sep 22, 2023
36 of 37 checks passed
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.

4 participants