-
-
Notifications
You must be signed in to change notification settings - Fork 239
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
feat: support jiff types #364
base: master
Are you sure you want to change the base?
Conversation
Signed-off-by: tison <wander4096@gmail.com>
schemars/tests/integration/jiff.rs
Outdated
// test!(Zoned) | ||
// .assert_allows_ser_roundtrip_default() | ||
// .assert_matches_de_roundtrip(arbitrary_values()); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Failed locally when running test:
failures:
---- jiff::jiff stdout ----
thread 'jiff::jiff' panicked at schemars/tests/integration/test_helper.rs:193:13:
serialize schema should allow serialized value: "1970-01-01T00:00:00+00:00[UTC]"
note: run with `RUST_BACKTRACE=1` environment variable to display a backtrace
failures:
Not sure if we need to extend the test helper but I'd include the minimal work code. Other improvements can be made as follow-ups.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Fixed in 3855539
- Use a custom format because jiff's
Zoned
default format is not "date-time". I'd like to know where is the definition of "date-time" though. - Enable default features for jiff in tests, it pulls the tzdb so that the test can work properly.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
While jiff's Zoned default serialize format is not fully RFC-3339 but instead "A hybrid format derived from RFC 3339, RFC 9557 and ISO 8601" - https://docs.rs/jiff/latest/jiff/fmt/temporal/index.html
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
To be clear here, specifically, jiff::Zoned
serializes to a fully compatible RFC 9557 timestamp. RFC 9557 is an update to RFC 3339.
Additionally, a jiff::Timestamp
serializes to a fully compatible RFC 3339 timestamp.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Additionally, a jiff::Timestamp serializes to a fully compatible RFC 3339 timestamp.
Yes. I tag jiff::Timestamp
with format: 'date-time'
and it works well.
jiff::Zoned serializes to a fully compatible RFC 9557 timestamp
Good to know. IIUC before JSON Schema provide a builtin format for it, or extend the date-time
format to accept RFC 9557, we should be fine with the workaround for a custom format. Out of curiosity, is the timestamp format in RFC-3339 a subset of RFC-9557's?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I think that makes sense. I'm not super familiar with JSON Schema though. Where is the custom format you're using documented? Like I would kind of expect to see a mention of RFC 9557 somewhere.
Out of curiosity, is the timestamp format in RFC-3339 a subset of RFC-9557's?
I would say yes. Although whether RFC 9557's clarification on the meaning of Z
and -00:00
is considered a superset of RFC 3339 is perhaps up for debate, but it's a very small point. In terms of the actual format supported, yes, RFC 9557 is a superset. Basically, RFC 9557 adds and defines the semantics for annotations to be appended to an RFC 3339 timestamp.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Where is the custom format you're using documented?
It's customized ("zoned-date-time") in this lib rather than documented elsewhere. I filed json-schema-org/json-schema-spec#1572 to see if the JSON Schema working group agrees to extend the definition of "date-time" format to accept timestamp strings in RFC 9557 format.
Signed-off-by: tison <wander4096@gmail.com>
This closes #359.
cc @GREsau
FYI @BurntSushi - I'd appreciate it if you have some time to see if this integration make correct assumption. And I noticed that jiff's weekday doesn't implement ser/de while chrono's does, perhaps it's a new feature candidate to consider.