Update validation section for ChilliCream Date/Time scalars#67
Update validation section for ChilliCream Date/Time scalars#67glen-84 wants to merge 2 commits intographql:mainfrom
Conversation
| - Fractional seconds, if present, are numeric (up to 9 digits) | ||
| - Fractional seconds, if present, are numeric and do not exceed the precision | ||
| supported by the implementation's underlying date-time type. Implementations | ||
| must not silently truncate or round fractional seconds beyond their supported | ||
| precision; instead, they should reject input that exceeds it. |
There was a problem hiding this comment.
I think this is a breaking change. The specifications should be implementation agnostic.
I interpret that sentence as "fractional seconds may have infinite precision" if an implementation supports it.
Which contradicts the counter example: 2023-12-24T15:30:00.1234567890Z might become a valid scalar representation after that change.
More generally, I think a scalar specification must not reference any specific implementation. It should stick to defining the format for all implementations.
There was a problem hiding this comment.
I interpret that sentence as "fractional seconds may have infinite precision" if an implementation supports it.
The maximum of 9 is specified for results, but you're right that this new text for inputs should also mention this limit. I'll update the text.
More generally, I think a scalar specification must not reference any specific implementation. It should stick to defining the format for all implementations.
Where is it referencing a specific implementation? This rule is for all implementations, intentionally.
If we specify 9 digits, then it might look like the Hot Chocolate implementation is invalid (only supporting 7), but maybe it's enough that the current spec uses "up to 9", and this update is unnecessary?
There was a problem hiding this comment.
Sorry, I meant its mentioning "implementations" in general. "implementations" leaves the door open to interpretations.
The specification should only care about describing the format. "Implementations" is a separate concern IMO.
f we specify 9 digits, then it might look like the Hot Chocolate implementation is invalid (only supporting 7)
I'd argue it is invalid then. Unless you explicitely allow loss of precision in input coercion, which sounds a bit dangerous but possible I guess.
There was a problem hiding this comment.
Or you may explicitely throw in input coercion if precision is too high I guess. All in all, I'd keep the "up to 9" and "input coercion: implementations must not lose precision and raise an error if the given value does not exist in their internal representation"
There was a problem hiding this comment.
"input coercion: implementations must not lose precision and raise an error if the given value does not exist in their internal representation"
... but this mentions implementations?
Also, did you mean must or must not raise an error? (slightly ambiguous in that sentence)
There was a problem hiding this comment.
Sorry it's the end of the week... 😅 It looks like you want to "raise an error" if the implementation does not support the value.
My beef is with this sentence:
- Fractional seconds, if present, are numeric and do not exceed the precision
supported by the implementation's underlying date-time type.
How many digits is that? No one knows from reading the spec. It's not describing a format, it leaves the format open to interpretation. I would instead go with either:
- fixed precision (9 digits for an example)
- infinite precision
Then if you really want to, you could add a note that implementations may throw in input coercion. That, at least leaves output coercion untouched.
Doing so kind of limits the value of the specification IMO, since it means some "valid" representations of the scalars won't be usable at runtime for some implementations.
If this is the format used by Hot Chocolate, I would go with fixed precision (7 digits max).
Another question is whether this requires a new url for people who are relying on the previous version of the scalar.
This is all FYI by the way, since you own this spec, feel free to land on what you think is best!
There was a problem hiding this comment.
I was initially trying to make the specs as "generic" as possible, and thus specified up to 9 fractional seconds for implementations that may support that. However, given that (a) These are now more ChilliCream-specific specs, and (b) .NET and our implementations only support 7 fractional seconds, it probably makes sense for the specs to also limit the digits to 7.
I've made these changes now, but let's wait until next week before we merge.
We don't need new URLs, since the existing URLs have not been used in a stable release of Hot Chocolate.
Thanks for the review. 🙂
There was a problem hiding this comment.
Sounds good 👍
the existing URLs have not been used in a stable release of Hot Chocolate.
Technically speaking, they're published on the internet so someone outside chillicream might already be relying on 9 digits somewhere in the world.
It's probably fine to not be too dogmatic here though.
2 participants
No description provided.