Fix SameResponseShape definition
#451
Conversation
|
Thank you for your pull request and welcome to our community. We require contributors to sign our Contributor License Agreement, and we don't seem to have you on file. In order for us to review and merge your code, please sign up at https://code.facebook.com/cla. If you are contributing on behalf of someone else (eg your employer), the individual CLA may not be sufficient and your employer may need the corporate CLA signed. If you have received this in error or have any questions, please contact us at cla@fb.com. Thanks! |
|
Thank you for signing our Contributor License Agreement. We can now accept your code for this (and any) Facebook open source project. Thanks! |
spec/Section 5 -- Validation.md
Outdated
| {mergedSet} including visiting fragments and inline fragments. | ||
| * Given each pair of members {subfieldA} and {subfieldB} in {fieldsForName}: | ||
| * If SameResponseShape(subfieldA, subfieldB)} is false, return false, | ||
| otherwise continue. |
There was a problem hiding this comment.
you don't need to write otherwise continue.
Example:
http://facebook.github.io/graphql/draft/#AreTypesCompatible()
If variableType is NOT a non‐null type, return false
spec/Section 5 -- Validation.md
Outdated
| * {SameResponseShape(subfieldA, subfieldB)} must be true. | ||
| * If {typeA} and {typeB} are the same type return true, otherwise return | ||
| false. | ||
| * If {typeA} and {typeB} are both composite types: |
There was a problem hiding this comment.
I suggest changing it to:
If {typeA} or {typeB} is not a composite type, return false
It allows moving subsequent steps on the top level and also removing final:
Return false
|
To achieve full consistency I would also change:
to
Same here:
|
|
Addressed review! |
spec/Section 5 -- Validation.md
Outdated
| * Assert: {typeA} and {typeB} are both composite types. | ||
| * If {typeA} and {typeB} are the same type return true, otherwise return | ||
| false. | ||
| * If {typeA} and {typeB} is not a composite type, return false. |
spec/Section 5 -- Validation.md
Outdated
| * Let {mergedSet} be the result of adding the selection set of {fieldA} and | ||
| the selection set of {fieldB}. | ||
| * Let {fieldsForName} be the set of selections with a given response name in | ||
| {mergedSet} including visiting fragments and inline fragments. | ||
| * Given each pair of members {subfieldA} and {subfieldB} in {fieldsForName}: | ||
| * {SameResponseShape(subfieldA, subfieldB)} must be true. | ||
| * If SameResponseShape(subfieldA, subfieldB)} is false, return false. |
There was a problem hiding this comment.
You missed the curly brace here
|
Great catch and fix! Thanks for your help |
leebyron
added
🤷♀️ Ambiguity
For motivation, see discussion in #450