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

add optional rkyv support #644

Merged
merged 2 commits into from
Mar 23, 2022
Merged

add optional rkyv support #644

merged 2 commits into from
Mar 23, 2022

Conversation

dovahcrow
Copy link
Contributor

@dovahcrow dovahcrow commented Jan 20, 2022

Thanks for contributing to chrono!

  • Have you added yourself and the change to the changelog? (Don't worry
    about adding the PR number)
  • If this pull request fixes a bug, does it add a test that verifies that
    we can't reintroduce it?

Fixes #583

@dovahcrow dovahcrow mentioned this pull request Jan 20, 2022
@djc
Copy link
Member

djc commented Mar 23, 2022

This seems to have a lot of unrelated of changes. I'm afraid the changes in #661 have caused quite a few conflicts, but if you would rebase this on current main and make only the minimal changes necessary to support rkyv I'd be happy to review this.

@dovahcrow
Copy link
Contributor Author

@djc Done.

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.

Thanks for rebasing!

src/date.rs Outdated
@@ -18,7 +18,8 @@ use crate::naive::{self, IsoWeek, NaiveDate, NaiveTime};
use crate::offset::{TimeZone, Utc};
use crate::DateTime;
use crate::{Datelike, Weekday};

#[cfg(feature = "rkyv")]
use rkyv::{Archive, Deserialize, Serialize};
Copy link
Member

Choose a reason for hiding this comment

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

Please put this in a separate block of imports, between the std/core block and the crate-internal imports. (Same for all the other imports.)

Copy link
Contributor Author

@dovahcrow dovahcrow Mar 23, 2022

Choose a reason for hiding this comment

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

do you want me to fix existing incorrect import orders as well? Some files are already not obeying this, e.g. src/naive/date.rs

Copy link
Member

Choose a reason for hiding this comment

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

It's not necessary for me to merge this PR, but if you don't mind doing the work that would be great! Please do it as a separate commit.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

done

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.

LGTM, thanks!

@djc djc merged commit c2e9f61 into chronotope:main Mar 23, 2022
@esheppa esheppa mentioned this pull request Jul 27, 2022
@jtmoon79 jtmoon79 mentioned this pull request Jan 23, 2024
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.

rkyv Support
2 participants