DSL implementation of fragments

[Cito]

This is my suggestion for #198

[codecov-commenter]

Codecov Report

Merging #235 (7354976) into master (7236bbd) will not change coverage.
The diff coverage is 100.00%.

@@            Coverage Diff            @@
##            master      #235   +/-   ##
=========================================
  Coverage   100.00%   100.00%           
=========================================
  Files           16        16           
  Lines         1139      1214   +75     
=========================================
+ Hits          1139      1214   +75     

Impacted Files	Coverage Δ
gql/cli.py	100.00% <ø> (ø)
gql/dsl.py	100.00% <100.00%> (ø)

---

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update 7236bbd...7354976. Read the comment docs.

[chadfurman]

I read through and it looks solid to me. Working on setting up a local env to play around with it a bit -- I've not yet done any contrib work on this repo. Thank you for your time and your quick response!

[vedantpuri]

Plus one to what Chad said! We really appreciate your quick response and time on this PR!

[leszekhanusz]

Hi @Cito, thanks for this PR.

I have some remarks:

• shouldn't we use DSLInlineFragment instead of DSLFragment in case we want to support normal fragments in the dsl module later ?
• DSLFragment inherits from DSLField but is not really a normal field (does not support alias or arguments). So you need to ignore the ast_field type which is not valid anymore. It would be more structured to create a new abstract class DSLSelection with the select method and have DSLField and DSLFragment inherit from it. What do you think ?

[Cito]

Hi @leszekhanusz.

• You're right - for a really clean implementation, I think it would be best to use a DSLSelection base class with two subclasses DSLField and DSLFragment, instead of abusing the existing DSLField for fragments.
• Not sure if the DSL should really expose different objects for inline and named fragments. The distinction could be actually left transparent to the user, e.g. the dsl_gql function could optimize automatically by using named fragments instead of inline fragments. For the users of the DSL it makes no difference, since they can create fragment objects once and reuse them anyway, without needing to explicitly create named fragments.

Sorry, I must leave it to you or others to work out the details due to lack of time. I just wanted to provide a rough idea how this can be implemented.

[leszekhanusz]

Sorry, I must leave it to you or others to work out the details due to lack of time. I just wanted to provide a rough idea how this can be implemented.

Ok, no worries, I'll propose something.

[chadfurman]

I also would be interested in helping if the work isn't all done at this point

[chadfurman]

Checking in to avoid duplicate efforts. I'm setting up my local env right now and going to read over the code in d505962 plus try and get a basic inline fragment to run + run the tests + evaluate the above comment against the commit.

[leszekhanusz]

@chadfurman
Now that I've started working on this, I would like to finish it. I hope you don't mind.

I am currently looking at how we could also support normal fragments into this PR.

That's when I realized that the DSLSelection class was actually used for two different purposes:

• for elements which can be arguments of the select method (selectable elements)
• for elements on which we can use the select method (containers)

So I decided to split it into DSLSelectable and DSLSelector.

Next I am going to introduce the FragmentSpreadNode and FragmentDefinitionNode from graphql-core AST.

[chadfurman]

Not a problem! Ping me if you want help on anything -- this or another ticket. For now, I'll switch tasks

Thank you for your effort and time :)

fwiw: I have my local env setup and all dev deps on the right version now w/ tests and make check all fine 😅

You have a very solid project here. Great work.

[leszekhanusz]

Not a problem! Ping me if you want help on anything -- this or another ticket. For now, I'll switch tasks

Thank you for your effort and time :)

fwiw: I have my local env setup and all dev deps on the right version now w/ tests and make check all fine sweat_smile

You have a very solid project here. Great work.

Thanks! If you want something to do, you could maybe check the issue #125, diving into the Appsync protocol and creating another transport AWSAppsyncTransport which inherits WebsocketsTransport.

[leszekhanusz]

* Not sure if the DSL should really expose different objects for inline and named fragments. The distinction could be actually left transparent to the user, e.g. the dsl_gql function could optimize automatically by using named fragments instead of inline fragments. For the users of the DSL it makes no difference, since they can create fragment objects once and reuse them anyway, without needing to explicitly create named fragments.

@Cito I tried to combine normal and inline fragments in a single class but they are really different beasts, with normal fragments which need a name and can have variables.
With my current code, if you use a DSLFragment in dsl_gql, you'll get a FragmentDefinitionNode, but if you use it as an argument of a select, then you get a FragmentSpreadNode

So now we need to know when we use a fragment as an argument in a select whether this is a normal fragment, and in this case we return a FragmentSpreadNode or whether it is an inline fragment and we return an InlineFragmentNode. If we used the same class, we could detect it somehow by checking if the fragment has a name, but it might get confusing and I think it is better that we stay explicit here.

[Cito]

with normal fragments which need a name and can have variables

But as far as I see, fragments with vars were experimental only (not part of the spec) and have now been deprecated in Graphql.js (and hence in GraphQL-core as well).

[leszekhanusz]

Interesting... But the main problem remains and it feels cleaner to me to keep the classes separate.
What do you think of the current state of this PR? Is this something we could agree on?

[Cito]

LGTM, it's definitely cleaner and allows more control over the generated query.

[leszekhanusz]

Good! I'll add some documentation and merge this PR next weekend.

[vedantpuri]

@leszekhanusz Any updates on this ?