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 Jiff date/time types Zoned/TimeZone #1278

Merged
merged 26 commits into from
Sep 6, 2024
Merged

Conversation

sgoll
Copy link
Contributor

@sgoll sgoll commented Aug 15, 2024

Description

This is a follow-up to #1271 and adds integration for the following Jiff date/time types:

The new types are hidden behind feature flag jiff-tz because they require enabling the std feature flag for the jiff 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 in jiff time zone features.

Closes #1270

Footnotes

  1. 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.

  2. This GraphQL scalar does not exist yet either. It is the union of GraphQL scalars TimeZone and UtcOffset.

@tyranron
Copy link
Member

@sgoll

The new types are hidden behind feature flag jiff-tz because they require enabling the std feature flag for the jiff crate (which is not necessary for the remaining Jiff date/time types introduced in #1271).

I think this is redundant. juniper at the moment is not capable of acting in no_std, so you could just add that feature and don't bother.

@tyranron tyranron added this to the 0.17.0 milestone Aug 16, 2024
@tyranron tyranron added k::integration Related to integration with third-party libraries or systems feature New feature or request lib::jiff Related to `jiff` crate integration labels Aug 16, 2024
@sgoll
Copy link
Contributor Author

sgoll commented Aug 16, 2024

I think this is redundant. juniper at the moment is not capable of acting in no_std, so you could just add that feature and don't bother.

I removed the feature flag and merged the master branch into this PR. With the recent commit that removed the panic in TimeZone, and the additions to your integration test, this is now ready for review and/or merge, I think.

@sgoll sgoll marked this pull request as draft August 16, 2024 09:08
@sgoll sgoll marked this pull request as ready for review August 16, 2024 13:13
Copy link
Member

@LegNeato LegNeato left a 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 Show resolved Hide resolved
book/src/types/scalars.md Outdated Show resolved Hide resolved
@@ -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
Copy link
Member

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

Copy link
Contributor Author

@sgoll sgoll Aug 23, 2024

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).

5ec7e8c, 2bd000c

juniper/src/integrations/jiff.rs Outdated Show resolved Hide resolved
@sgoll
Copy link
Contributor Author

sgoll commented Aug 23, 2024

I am not sure whether the two failing tests in the last CI run are related to my changes.

@LegNeato LegNeato merged commit fb1531f into graphql-rust:master Sep 6, 2024
172 of 174 checks passed
@LegNeato
Copy link
Member

LegNeato commented Sep 6, 2024

Failures Re unrelated. Thanks again!

tyranron added a commit that referenced this pull request Sep 9, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
feature New feature or request k::integration Related to integration with third-party libraries or systems lib::jiff Related to `jiff` crate integration
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Provide support for Jiff date/time types
4 participants