Clarify that Float does not include NaN or infinity#780
Clarify that Float does not include NaN or infinity#780leebyron merged 3 commits intographql:mainfrom
Conversation
glasser
force-pushed
the
glasser/finite-floats
branch
from
acf270e to
df8039e
Compare
juriad
left a comment
juriad
left a comment
There was a problem hiding this comment.
On lines 474/475 integer is specifically mentioned, however a similar problem can happen with float: 1e1000. Do we want to change the formulation there too?
|
@juriad is this suggestion directly related to my change, or a separate opportunity for improvement that could be considered in parallel? (I'm not sure what the answer to your question is and am maybe hoping to not have to :) ) |
|
@glasser I assume that the mention of NaNs should be also removed from this section: https://spec.graphql.org/June2018/#sec-Value-Completion |
|
@fedinskiy What change do you think is relevant there? This seems consistent with that mention: that mention implies that a |
leebyron
added
the
✏️ Editorial
|
@glasser do you mind taking a look at the CLA bot? |
|
I think we need to readdress @fedinskiy's comment as well. In retrospect it seems wrong that |
Fixes graphql#778. This matches the fact that you cannot represent these values in text as FloatValue, as well as the graphql-js implementation (https://github.com/graphql/graphql-js/blob/16009cbcb0109da03f2157a868817b886801095a/src/type/scalars.js#L108-L112). This was perhaps already implied by the word "fractional", but "finite" seems to be a more standard term for "IEEE 754 floats that are not infinity or NaN".
leebyron
force-pushed
the
glasser/finite-floats
branch
from
b20c639 to
8a8f9dc
Compare
leebyron
force-pushed
the
glasser/finite-floats
branch
3 times, most recently
from
18d7924 to
0f46b62
Compare
leebyron
force-pushed
the
glasser/finite-floats
branch
from
0f46b62 to
6c7e0b6
Compare
| * Return {completedResult}. | ||
| * If {result} is {null} (or another internal value similar to {null} such as | ||
| {undefined} or {NaN}), return {null}. | ||
| {undefined}), return {null}. |
There was a problem hiding this comment.
This change is technically a normative change, not just editorial. However I did some checking and found the reference implementation (and most others) do not adhere to short-circuiting NaN to null.
They instead treat NaN as an invalid Float and throw an error, which is exactly the behavior now accurately described above.
There was a problem hiding this comment.
GraphQL.NET throws on NaN.
|
Having trouble with the CLA but will keep iterating with LFX support (https://jira.linuxfoundation.org/plugins/servlet/theme/portal/4/SUPPORT-4646). Your updates to my text seem reasonable (I haven't verified your claims about the behavior of existing implementations with NaN but believe you!). |
|
CLA fixed. I have used better web apps. |
|
Here's the relevant lines from GraphQL.js
Re: my inline comment above and the related #836 which clarifies that NaN should fail to coerce and throw an error, GraphQL.js Float coercion for inputs and outputs does in fact throw for any non-finite value |
|
Thanks! |
6 participants
Fixes #778.
This matches the fact that you cannot represent these values in text as
FloatValue, as well as the graphql-js
implementation (https://github.com/graphql/graphql-js/blob/16009cbcb0109da03f2157a868817b886801095a/src/type/scalars.js#L108-L112).
This was perhaps already implied by the word "fractional", but "finite" seems to
be a more standard term for "IEEE 754 floats that are not infinity or NaN".