add support for time::Time

[vnghia]

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]

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]

@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]

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]

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]

Thank you for your review. I add a ranged_int macro

[caibear]

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.