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 support for time::Time #43

Open
wants to merge 4 commits into
base: main
Choose a base branch
from
Open

Conversation

vnghia
Copy link

@vnghia vnghia commented Sep 20, 2024

Part of #37, I would like your opinion about this approach @finnbear. Thank you!

I move out Nanosecond to datetime so later it can be reused by other crate (for example #39)

I am not sure why but this line throw an error: assert!(crate::decode::<Time>(&crate::encode(&(23, 59, 59, 999_999_999))).is_ok()); when populate nanosecond. Could you check for me if there are any typo here :/ That is so weird

@finnbear
Copy link
Member

finnbear commented Sep 20, 2024

Thanks for the PR! I'm curious why you didn't use the ConvertFrom approach to reduce code duplication.

I am not sure why but this line throw an error: assert!(crate::decode::(&crate::encode(&(23, 59, 59, 999_999_999))).is_ok());

Could be because 23 and 59 will be i32, Rust's default type for ambiguous integer literals, instead of u8. In any case, consider encoding Time instead of a tuple (for valid cases) and adding Time to the fuzzer (for invalid ones).

@vnghia vnghia marked this pull request as ready for review September 21, 2024 13:12
@vnghia
Copy link
Author

vnghia commented Sep 21, 2024

@finnbear I've addressed your review. Do you think I should add support for Date in this PR as well (basically two construction blocks of the time crate) or should I send another PR ? Thank you

@finnbear
Copy link
Member

finnbear commented Sep 23, 2024

Thanks! As far as I'm personally concerned, there is no need to wait for implementing all types of a crate before merging any given PR. This PR looks good to me! 👍

But I want to here @caibear's opinion on supporting the time crate (and potentially chrono/jiff) in general. There are a few factors that make this PR less likely to be a concern:

  • limited additional unsafe, thanks to ConvertFrom encoder
  • #[derive(Encode, Decode)] inside time is currently out of the question, since it wouldn't support the limited-range fields
  • https://crates.io/crates/time is very popular, with 280M downloads at the time of writing

Only room for improvement that I can see is to make a macro to define a limited-range integer, such as Nanoseconds and Seconds, as there is some repetition there.

@vnghia
Copy link
Author

vnghia commented Sep 24, 2024

Hey @finnbear (and @caibear as well XD), speaking about macro, have you ever considered adding some internal proc-macro. Like ConvertFrom might be replaced with a proc-macro. It is just a vauge idea in my head so not for this PR but I prefer working with proc-macro more then macro-rules so just some suggestions.

@vnghia
Copy link
Author

vnghia commented Sep 24, 2024

Thank you for your review. I add a ranged_int macro

@caibear
Copy link
Collaborator

caibear commented Sep 24, 2024

Hey @finnbear (and @caibear as well XD), speaking about macro, have you ever considered adding some internal proc-macro. Like ConvertFrom might be replaced with a proc-macro. It is just a vauge idea in my head so not for this PR but I prefer working with proc-macro more then macro-rules so just some suggestions.

We have been considering using bitcode's derive macro on remote types by running it on a copy of the remote type declaration but replacing the local type with the remote type in the macro's output. This would allow implementing Encode/Decode easily on any type with all public fields such as Option,Result,IpAddr.

The main roadblock is that currently with 0.6, feature="derive" is not required to use bitcode::encode/decode so we'd either have to make bitcode_derive a required dep for the remainder of 0.6 or wait for 0.7.

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.

3 participants