Implement BuildClientSchema #539
Conversation
|
@simPod it fit better with the rest of the library, which also uses associative arrays for other parts that interface with client code. I prefer |
|
Cool, let me know when you finish. |
|
@vladar finished the implementation and test suite. A few minor fixes in the rest of the library came up while implementing the whole test suite. No changes to other tests were required and i don't think there are breaking changes. I stuck as close as possible to the reference implementation. There were 2 tests i had to skip, related to #567 |
|
Awesome, I'll do a full review as soon as I have a chance! Quick note - looks like style checks are failing |
|
@vladar everything is passing now apart from Scrutinizer complaining about the complexity of I think we should stick close to the reference implementation and keep the class as is. The error should go away once this is merged, right? |
simPod
force-pushed
the
master
branch
2 times, most recently
from
61b0907 to
8d37049
Compare
# Conflicts: # composer.json
|
@spawnia Ignore scrutinizer, we'll remove it from our checks and just use PHPStan. I am ready to merge this but can you please resolve conflicts first? |
|
@vladar OT: I marked only Travis as a Required check few weeks ago. |
|
Finally, the pipeline succeeded. @vladar this should be good to go. I have been using this branch successfully in https://github.com/spawnia/sailor |
| /** | ||
| * @see BuildClientSchema | ||
| */ | ||
| class BuildClientSchemaTest extends TestCase |
There was a problem hiding this comment.
final, should not be allowed to be extended
There was a problem hiding this comment.
I am firmly opposed to the usage of the final keyword in libraries such as this.
There was a problem hiding this comment.
I understand there are people avoiding final but for tests there's no issue, hm? Consumers won't get affected anyway and we accidentally won't extend Tests nor they will get suggested in autocomplete.
Co-Authored-By: Šimon Podlipský <[email protected]>
Co-Authored-By: Šimon Podlipský <[email protected]>
src/Utils/Utils.php
Outdated
| @@ -272,9 +272,9 @@ public static function groupBy($traversable, callable $keyFn) | |||
| } | |||
|
|
|||
| /** | |||
| * @param mixed[]|Traversable $traversable | |||
| * @param iterable $traversable | |||
There was a problem hiding this comment.
Fixed all related type hints in #613
|
Awesome! Thank you for this huge work. And sorry for being late to merge this - last two months were pretty intense for me. |
Resolves #293
Equivalent to https://github.com/graphql/graphql-js/blob/master/src/utilities/buildClientSchema.js
@vladar is there something you wish to change about the implementation? I would do the tests next.