Closed excessdenied closed 4 years ago
Thanks for the PR! I haven't gone through it with a fine-toothed comb yet but it looks to me like the bulk of the existing code has been moved to the objecttype
module, and the new inputobjecttype
module is largely copied from there. For example, unless I'm missing something, the converters
and registry
modules are almost the same.
I'd personally prefer not to copy code like this but find ways to reuse it if possible. For example, could we simply use the same converter functions and registry for both PydanticObjectType
and PydanticInputObjectType
?
Yes, the objecttype package should have no changes to the original code. This should be a purely additive change in terms of code paths.
Because ObjectType/Field and InputObjectType/InputField are separate type hierarchies in Graphene and GraphQL, there are some nuances that make them hard to generic-ize. I've found at least Inputs not supporting Unions and Interfaces, but since I'm new to GraphQL, Graphene and Pydantic, there may be other issues I'm not aware of.
Most importantly, I'd rather not break the existing ObjectType functionality that your production systems depend on, just to save some code. I'd rather have the new functionality available to people who want it, and we can do more serious refactoring later.
Most importantly, I'd rather not break the existing ObjectType functionality that your production systems depend on, just to save some code.
Thank you for thinking about not breaking existing production code! I really appreciate that. I want to talk some more about that next sentence, though:
I'd rather have the new functionality available to people who want it, and we can do more serious refactoring later.
I understand where you're coming from, and thank you for the work! Please understand I'm not criticizing the work, just coming at it from a different perspective.
For me the cost of maintaining duplicated code is high (as a repo owner who will have to do the refactoring), and the benefit is low (since I don't personally have a need for InputObjectType
s in my code, and only one other user has expressed a need for this (#19)). So I'd hope we can do more refactoring in this PR.
Specifically:
inputobjecttype/registry.py
vs objecttype/registry.py
the only differences I can see are PydanticObjectType
vs PydanticInputObjectType
. I'd suggest that all the otherwise identical lines should stay in a base class, and two subclasses could describe the different behavior of the two different field registries. inputobjecttype/types.py
and objecttype/types.py
are similar, and we could share functionality similarly, but am less certain about that since these are whole type definitions here.inputobjecttype/converters.py
vs objecttype/converters.py
, I find that the convert_pydantic_type
and find_graphene_type
functions are identical, and convert_generic_python_type
differs only in its lack of Union
support. I would suggest that at least convert_pydantic_type
and find_graphene_type
can be shared entirely, and convert_generic_python_type
could take an argument for union support. The version for InputObjectTypes
would call it with the argument set to False
.I've generic-ized Registry, and shared the converters logic. Union failures in InputObjectTypes will be handled upstream by GraphQL's code, and I've added a note in the README as to the possible error message. Please take a look.
@excessdenied thank you! This looks much easier to manage -- I'll have a look over the weekend!
Thank you again @excessdenied for this -- note that this feature has been released in v0.1.0 on PyPI
You're welcome, and thanks for the review :)
Title
Please describe the feature(s) added, bug(s) fixed, etc here at a high level, and go into detail in further paragraphs if necessary.
Upside CLA