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

impl GraphQLScalar for NaiveTime #657

Merged
merged 2 commits into from
May 21, 2020
Merged

Conversation

c410-f3r
Copy link
Contributor

No description provided.

@LegNeato
Copy link
Member

Thanks for the PR! I'm actually not sure we want to land this. One can imagine different users needing different time representations (e.g. 1:00pm vs 13:00:00, 09:15:00 vs 09:15, etc). Need to think about it some more.

@tyranron
Copy link
Member

@LegNeato despite there is nothing in spec about it yet, I suppose it's naturally to follow RFC 3339 time representation for a default library scalar.

If somebody needs some custom time formatting, he always can go with a custom scalar.

@LegNeato
Copy link
Member

Yeah, we do that for other date/time stuff. I guess it is not clear what context NaiveDateTime is used in so it is less clear there is a default way to represent or even what the data NaiveDateTime represents in most contexts.

I'm inclined to merge this but I feel it moves Juniper to be more opinionated and less universal. Though I guess an argument can be made with uuid and dashes vs no dashes. But in that case at least the underlying data domain is the same.

@c410-f3r
Copy link
Contributor Author

If following an hypothetical or concrete standard is a hard constraint, then it is possible to write these "non-standard" implementations behind a feature flag, say "extra", "non-standard-impls" or "shinigamis". By doing so, you guys will take off the burden of someone that must create a wrapper and implement all by himself due to coherence rules, plus avoiding the spread of duplicated code across different projects.

@LegNeato
Copy link
Member

Let's put it behind a flag, off by default. Let's use the prefix scalar- so future ones can follow the same format and people can turn on whatever combo they want.

@c410-f3r
Copy link
Contributor Author

@LegNeato Thank you

@LegNeato LegNeato merged commit 2cb96d0 into graphql-rust:master May 21, 2020
@LegNeato
Copy link
Member

Thanks for the PR and adding the flag and tests!

@c410-f3r c410-f3r deleted the naivetime branch May 21, 2020 11:34
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants