-
Notifications
You must be signed in to change notification settings - Fork 1
feat: Add 'UTC-NOW' support to DateTime #15
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
Conversation
|
I like this feature. Glad to see it being added. @fredcarle @jsimnz I think we had a DevEx call a couple of weeks ago where we discussed the idea of having certain keywords follow the ALLCAPS style of naming (leaning into how SQL does it), instead of the If that still holds, I might suggest it should be |
AndrewSisley
left a comment
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.
Got a couple of comments that I'd like sorted before merge, and the following thought:
thought: This does feel a little incorrect, as the DateTime is a scalar, and now is essentially a function. I cannot think of anything that will break for sure right now, including ORMs though, as I believe the GQL types are only ever input-types. I have a bit of a worry that this might result in more complicated type-mapping for ORMs, especially for typed langs that don't support union types.
The flip side is, this feature is otherwise really nice, and applies to basically everywhere without us having to worry about it or do much coding :)
scalars.go
Outdated
| return unserializeDateTime(valueAST.Value) | ||
| case *ast.EnumValue: | ||
| // Support the literal `now` | ||
| if valueAST.Value == "now" { |
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.
suggestion: "now" is ambiguous IMO, "utc-now" would be clearer I think, and allows us to add other variants more cleanly without changing this definition.
| case *ast.StringValue: | ||
| // Parse normal RFC3339 string | ||
| return unserializeDateTime(valueAST.Value) | ||
| case *ast.EnumValue: |
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.
todo: Github wouldn't let me comment on un-edited lines - please document this in the Description a few lines up, please make sure you note the time zone or lack-of, and that it is evaluated server-side as that can be a bit ambiguous when using some clients.
Thanks for the input. I won't rush to merge it. I think maybe this issue, as well as exactly what to call the field, could be good to discuss at the next standup. |
This PR makes a small adjustment to the
DateTimescalar, to allow use of a literal value,nowwhich will be parsed into the current time at the time of resolution.This will, for example, allow us to do a query like the following, over on the Defradb side of things:
Notice that
nowis not a string, but is a literal that gets handled.This PR is proposed as the first step in resolving this issue over on Defradb:
sourcenetwork/defradb#4152
My intention is to add integration tests of this functionality on the Defradb side of things.