Skip to content
This repository has been archived by the owner on Mar 8, 2023. It is now read-only.

Execution fragments deeper #25

Merged
merged 4 commits into from
Mar 5, 2018
Merged

Execution fragments deeper #25

merged 4 commits into from
Mar 5, 2018

Conversation

hungneox
Copy link
Contributor

@hungneox hungneox commented Mar 5, 2018

I have to improve the implementation a lot more but basically now the execution can execute multiple fragments.
It's done in a recursive way which I think not so good. I will try to do it in non-recursive way.

@hungneox hungneox requested a review from crisu83 March 5, 2018 01:29
@coveralls
Copy link

coveralls commented Mar 5, 2018

Pull Request Test Coverage Report for Build 214

  • 33 of 37 (89.19%) changed or added relevant lines in 3 files are covered.
  • No unchanged relevant lines lost coverage.
  • Overall coverage increased (+0.09%) to 84.741%

Changes Missing Coverage Covered Lines Changed/Added Lines %
src/Execution/ExecutorExecutionStrategy.php 0 1 0.0%
src/Execution/ExecutionResult.php 0 3 0.0%
Totals Coverage Status
Change from base Build 208: 0.09%
Covered Lines: 1927
Relevant Lines: 2274

💛 - Coveralls

Copy link
Contributor

@crisu83 crisu83 left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Looks good to me. I added a few comments though.

*/
private function getFieldNameKey(FieldNode $node)
{
return $node->getAlias()
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

We should a hasAlias method to the associated trait and use that instead.


$result = $this->resolveField($parentType,
$source,
[],
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

We don't support arguments yet?

Copy link
Contributor Author

@hungneox hungneox Mar 5, 2018

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

We do support args

$inputValues = $fieldNode->getArguments() ?? [];
$args        = [];

foreach ($inputValues as $value) {
    if ($value instanceof ArgumentNode) {
        $args[] = $value->getValue()->getValue();
    } elseif ($value instanceof InputValueDefinitionNode) {
        $args[] = $value->getDefaultValue()->getValue();
    }
}

$fields
);

$subResult = array_merge_recursive(
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This can be a costy operation. We should add a TODO to benchmark it.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I'm going to refactor this very soon :D

$fields = $this->collectFields(
$parentType,
$fieldNode->getSelectionSet(),
new \ArrayObject(),
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Why not use [] instead?

@hungneox hungneox merged commit bd3178c into master Mar 5, 2018
@hungneox hungneox deleted the execution-fragments-deeper branch March 5, 2018 09:57
@crisu83 crisu83 added this to the Version 1.0 milestone Oct 2, 2018
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

3 participants