-
Notifications
You must be signed in to change notification settings - Fork 425
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 Jiff date/time types Zoned
/TimeZone
#1278
Conversation
I think this is redundant. |
I removed the feature flag and merged the |
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.
Some minor nits. I'll give @tyranron a chance to look over but LGTM overall! Thanks for putting up such high quality PRs.
book/src/types/scalars.md
Outdated
@@ -424,6 +430,7 @@ mod date_scalar { | |||
[`chrono::NaiveTime`]: https://docs.rs/chrono/latest/chrono/naive/struct.NaiveTime.html | |||
[`chrono-tz`]: https://docs.rs/chrono-tz | |||
[`chrono_tz::Tz`]: https://docs.rs/chrono-tz/latest/chrono_tz/enum.Tz.html | |||
[`rust_decimal::Decimal`]: https://docs.rs/rust_decimal/latest/rust_decimal/struct.Decimal.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.
Is looks unrelated and there is already Decimal
below
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.
Thank you for pointing this out. Originally, I noticed that the Rust type rust_decimal::Decimal
in the table with the supported scalars had no link and added it.
But after your comment I now notice that the existing link seems to be unused. I have removed it (or changed its label to the one used in the table, if you want to think about it this way).
Co-authored-by: Christian Legnitto <[email protected]>
I am not sure whether the two failing tests in the last CI run are related to my changes. |
Failures Re unrelated. Thanks again! |
Description
This is a follow-up to #1271 and adds integration for the following Jiff date/time types:
Zoned
to GraphQL scalarZonedDateTime
1tz::TimeZone
to GraphQL scalarTimeZoneOrUtcOffset
2, and to GraphQL scalarTimeZone
with a newtypetz::Offset
to GraphQL scalarUtcOffset
The new types are hidden behind feature flagjiff-tz
because they require enabling thestd
feature flag for thejiff
crate (which is not necessary for the remaining Jiff date/time types introduced in #1271).In addition, the
jiff
crate itself must be installed either with the default feature flags or with explicit feature flags that make the Time Zone Database available as explained injiff
time zone features.Closes #1270
Footnotes
This GraphQL scalar is not documented at https://graphql-scalars.dev but we can introduce it here as laid out in https://github.com/graphql-rust/juniper/issues/1270#issuecomment-2291041828. ↩
This GraphQL scalar does not exist yet either. It is the union of GraphQL scalars
TimeZone
andUtcOffset
. ↩