Adding an extension field to the ExecutionResult

[Cito]

In GraphQL.js > 15.1.0 the ExecutionResult now has an optional extensions field, and I wonder how this should be best implemented in Python.

So far, ExtensionResult has been a named tuple with the two fields data and error. Should we now:

• add a third field extensions to the named tuple?
• or rather use a data class for ExtensionResult instead of a named tuple?

A problem with the first solution is that the named tuple now becomes a bit clumsy and verbose, the extensions fille would then always be printed as part of the __repr__() even if it is not used, and it would be mandatory in the __init__() signature. Unfortunatelly, it's also not possible to overwrite the __init__() method of named tuples to make the third argument optional.

A problem with the second solution is that real data classes are only available in Python 3.7, but we currently still support Python 3.6. It's not a big problem, since we could use a normal class and add the data class methods manually, or make use of the "dataclasses" package for Python 3.6.

A problem with both is that it will cause some backward incompatibility, e.g. in comparisons like

result == (data, errors)

or assignments with unpacking like

data, errors = result

We could solve the first by using a custom __eq__ method that would allow comparisons with 2- or 3-tuples. The second could be solved by making it an iterable with two elements. But then it would not be possible to do

data, errors, extensions = result

So probably better not make it an iterator, because "In the face of ambiguity, refuse the temptation to guess."

I would be glad to get some feedback from the community here.

[syrusakbary]

In Graphene we are using a dataclass port when used in Python < 3.6.
What about going with choice 2, but with a backport for older Python versions?

I think w can still allow unpacking the way we were doing before

data, errors = result

[Cito]

I'm also leaning towards that solution. To allow the unpacking, we would need to make it appear like an iterable with 2 elements, right? It would be possible, but it feels a bit strange to return 2 elements, while astuple() would return 3. But maybe it's ok for the sake of simplicity and backward compatibility.

[Cito]

Follow-up questions if we use a data class:

• Should we make it immutable (frozen=True) and use __slots__?
• Should we set compare=True and hash=True include for extensions?

[KingDarBoja]

Would be good to link where ExtensionResult is located at the source implementation, as I have no idea what does this new extensions field to give my opinion about how it should be implemented :)

EDIT 1 Looks like it is mentioned on the spec doc https://spec.graphql.org/draft/#sec-Response-Format
EDIT 2 Found it: https://github.com/graphql/graphql-js/blob/master/src/execution/execute.d.ts#L49

Based on a quick read, I think the idea of using dataclass and provide a workaround to unpack it as 2-element tuple is the way to go as this extensions field seems to be undocumented at the time being and I don't think any user should be using it at the moment.

[Cito]

@KingDarBoja, yes that's the right location and passage in the specs.

I have converted it to a class now that can be unpacked and compared as a 2-tuple and a dict. I did not use a dataclass decorator, since the methods had to be created manually anyway to achieve that backward compatibility. Also dataclasses do not support slots when they have default elements. After this change. the full test suite still passed without any adaptation.