Add serde support to date time #2646
Conversation
thomas-k-cameron
force-pushed
the
RFC30/date_time
branch
from
258f754
to
bda4e51
Compare
There was a problem hiding this comment.
Looks good. Just some minor things.
Could you also add a cargo invocation to this script that runs tests with aws_sdk_unstable and the two serde features enabled? https://github.com/awslabs/smithy-rs/blob/cbbbec4a4892e301ebde3af5cc852aa48d82e608/rust-runtime/aws-smithy-types/additional-ci#L7
|
I don't know where the error is coming from. The nightly compiler that you are using seems to be pretty old, maybe an update? |
|
OK, it's working now. |
| E: serde::de::Error, | ||
| { | ||
| match self.state { | ||
| VisitorState::Unexpected => fail(VisitorState::UNEXPECTED_VISITOR_STATE), |
There was a problem hiding this comment.
I don't think this compiles. Could you also add a cargo invocation to this script that runs tests with aws_sdk_unstable and the two serde features enabled? https://github.com/awslabs/smithy-rs/blob/cbbbec4a4892e301ebde3af5cc852aa48d82e608/rust-runtime/aws-smithy-types/additional-ci#L7
That should make CI test it.
There was a problem hiding this comment.
Thanks for letting me know!
I thought it is already covered by the existing tests.
I completely missed it.
There was a problem hiding this comment.
I figured out why it wasn't showing up any errors.
This branch doesn't have [features] key on Cargo.toml.
It seems like cargo just ignored anything that was feature-gated behind serde-serialize and serde-deserialize
|
What was the problem? Does it not call the different visit methods? We could just make them both |
|
You are right! It's working now. I don't know what I was thinking. |
thomas-k-cameron
force-pushed
the
RFC30/date_time
branch
from
8d1f648
to
4be97c3
Compare
|
No idea where linker error is coming from. |
|
I tried to reproduce this but it hasn't been working. Maybe something to do with the docker image or the instance? |
I've seen this happen as part of the minimal versions check we do. Tracing at v1.0 has some issue with an attribute macro, so when the minimal versions check downgrades tracing, this error can show up. I think we should start using a version of tracing that's newer. |
|
The latest version of tracing is v0.1.38. What do you mean by tracing v1.0? |
|
You don't need to worry about CI on the examples. That's just a notification to us so that we can fix the examples before doing a release. It doesn't block merge. The important CI job that must pass before merge is |
Cool. Thanks! |
| impl<'de> Visitor<'de> for NonHumanReadableDateTimeVisitor { | ||
| type Value = DateTime; | ||
| fn expecting(&self, formatter: &mut fmt::Formatter<'_>) -> fmt::Result { | ||
| formatter.write_str("Datat must be number") |
There was a problem hiding this comment.
Can you make this error message clearer?
| } | ||
|
|
||
| /// check for human redable format | ||
| #[test] |
There was a problem hiding this comment.
These should be in a #[cfg(test)] mod tests {
| } | ||
|
|
||
| /// check for human redable format | ||
| #[test] |
| } | ||
| } | ||
|
|
||
| #[test] |
There was a problem hiding this comment.
| #[test] | |
| #[cfg(test)] |
|
I recommend installing the pre-commit commit git hook to the git repo by running |
Actually, I have pre-commit but it takes for ever to finish (like 10 to 20 minutes). So I usually just push it and see if it works. I will make sure to pre-commit before I push it from now on 😄 |
Huh. That's odd. It takes about 10 seconds on my machine if the tools are already compiled, and maybe 2 minutes otherwise. |
Maybe because I'm using VSC on windows inside a docker. It was lot faster on github codespace so. |
Motivation and Context
This is a child PR of #2616
The changes that this PR introduces is same as the ones that were merged to
unstable-serde-supportbranch before.Initially, we tried to make commit to unstable-serde-support branch and merge changes one by one in small PRs. However, in order to make it up to date with the main branch, we would need to go through a large PR of over 700 files.
Thus, I decided to create individual PRs that commits directly to
mainbranch.Description
serdesupport toDateTimeTesting
Checklist
NA
By submitting this pull request, I confirm that you can use, modify, copy, and redistribute this contribution, under the terms of your choice.