Refactor/reduce code duplication in plugins

[j-riebe]

@jhnnsrs I think its time, that you take a look at this.

The PR contains the refactoring of EnumsPlugin, InputsPlugin and ObjectsPlugin.
I mainly focused on breaking down the large functions into smaller methods of each Plugin.
Some code duplication is eliminated, the remaining similarities should be pretty obvious by now (as a preparation for the next refactoring iteration).
There is a new module ast_generators that contains all concrete AST-Generation logic that was (duplicated a lot) in every plugin before.

I have the feeling, that there also should be a stricter separation between the concerns:

• Schema Parsing (Main job of the Plugin)
• Registration (Which Objects/Enums/Inputs to include)
• AST-Generation (Translate GraphQL Type to pydantic Model + Plugin-specific types like Operations/Funcs)
• Custom Styling (influence ast-generation, not during class registration)

Currently, each plugin does almost everything.
Only the styling is somehow part of the ClassRegistry, were it shouldn't belong IMHO.

Also, have a look at this test tests/plugins/test_objects.py::make_test_cases_union_field_value

[jhnnsrs]

Great work!

I especially appreciate the "TypeAstGenerator" efforts. That should easily translate to the other plugins as well, and I could see this potentially extend to having a "pure dataclass" or "attrs" version to allow support for generation other than pydantic. Great stuff!

I see we are also becoming fully object-oriented ;) Personally I am not sure if that is really increasing readability (e.g one-line functions like "_type_has_description"). But I am not too hard set on this. I am just wondering how well it will translate to more recursive plugins like fragments or operations (in terms of readability)?

Registration would be something that i would feel like should be handled by the plugin itself, but yes, maybe through composition of a 'Filter mixin' implementing "skip_type", according to some config parameters like

- type: enums.Enums.Plugin
  filter: 
       whitelist: 'regex'

That would maybe also justify defining config values directly in the plugin instead of a dedicated configclass.
What are your thoughts?

Again great work! Thanks a lot for putting so much work in it!

[jhnnsrs]

Whats your idea where styling should be placed? Closer to the ast generation itself?

[j-riebe]

I personally really like the idea, of directly passing the GraphQL-Core-Types to the Ast-Generator and make it an implementation detail of the generator what comes out.

Therefore, the stylers could / should be given to the ast-generator as a dependency.

But I'm not sure if that leads to difficulties when the styled-names are referenced ...
Maybe we just need a simple map(ping function) here, like: type_to_styled_name_map: Dict[GraphQLNamedType, str].

[j-riebe]

I see we are also becoming fully object-oriented ;) Personally I am not sure if that is really increasing readability (e.g one-line functions like "_type_has_description"). But I am not too hard set on this. I am just wondering how well it will translate to more recursive plugins like fragments or operations (in terms of readability)?

Well, I really think separating meaning and implementation improves the "thinkability" a lot. Even if it leads to more LoC.

The difference between:
if self._type_has_description(gql_type)
and
if gql_type.description is not None
are the 10 seconds (or more)

• to lookup, that gql_type is e.g. a GraphQLObjectType,
• that GraphQLObjectType has a description field
• the description can be optional
• and to figure out, that it should be "self-explanatory" that there is no description if description is None.

Compared to a simple Does it have a description?

Anyways, the main point was to put this method into a shared base class.

BTW, I'm not sure anymore if the base-Plugin is the right place for this. Maybe a composition (e.g.TypePluginMixin ) is better suited.

[j-riebe]

Registration would be something that i would feel like should be handled by the plugin itself, but yes, [...].

What is the purpose of ClassRegistry then?

If we take away styling - as this is a separate concern and should be a separate class then - there is not much left.

I can figure out:

• register imports (which is really nice)
• register forward references.

Did I miss something?

And one thought to the forward references:
Does it add any value not to use forward references everywhere?
It would reduce the ClassRegistry to a list-manager of imports and types that need to call XXX.update_forward_ref() at the end of the file (this could shrink the code to almost nothing instead of 400+ lines 😉 )

[jhnnsrs]

No thats about it for ClassRegistry ;)
I do however think that keeping track of forward references is useful, as every call to update_forward_refs has an impact
on module import, and that is something that pydantic already struggles with, so i would try to keep that as minimal as possible.

Maybe we just need a simple map(ping function) here, like: type_to_styled_name_map: Dict[GraphQLNamedType, str].

This I think would be interesting, even going so far as to say that maybe it should be

type_to_ast_element_map: Dict[GraphQLNamedType, AstElement]
where AstElement could representent the ast.Node that was generated through the AstGenerator(Factory) (and i agree here should we put the stylers) so we could have

class AstElement(ABC):
      
    
class UnionElement(AstElement):
   ''' Could be used for referencing Interfaces ''''
   pass

class ClassElement(AstElement).
   pass

And then according to the AstFactory an element like this.

class PydanticClass(ClassElement):
    pass

class DataclassClass(ClassElement):
    pass

And these would then be registered in the Registy?

[jhnnsrs]

I see we are also becoming fully object-oriented ;) Personally I am not sure if that is really increasing readability (e.g one-line functions like "_type_has_description"). But I am not too hard set on this. I am just wondering how well it will translate to more recursive plugins like fragments or operations (in terms of readability)?

Well, I really think separating meaning and implementation improves the "thinkability" a lot. Even if it leads to more LoC.

The difference between: if self._type_has_description(gql_type) and if gql_type.description is not None are the 10 seconds (or more)

• to lookup, that gql_type is e.g. a GraphQLObjectType,
• that GraphQLObjectType has a description field
• the description can be optional
• and to figure out, that it should be "self-explanatory" that there is no description if description is None.

Compared to a simple Does it have a description?

Anyways, the main point was to put this method into a shared base class.

BTW, I'm not sure anymore if the base-Plugin is the right place for this. Maybe a composition (e.g.TypePluginMixin ) is better suited.

I agree, there are definitely some benefits! And with a composition to TypePluginMixin I am now completely on board 😝

[jhnnsrs]

Would you be willing to continue on this PR until we have the transition of all plugins to be merged? Or do you feel it would be good to already preliminary merge?

[j-riebe]

I could imagine that, but I can't promise that I will get to it (focusing on the ariadne-plugin until now).

Keep in mind, that you are also able to push to this branch (in my fork), as it is a fork of your project.

As an alternative, we could merge this PR into a feature-branch in the main repo - to ease collaboration.

[jhnnsrs]

Hi @j-riebe , sorry for the late reply, i was knocked out cold by covid and the brain fog is just lifting :D There is some mature deadlines heading my way so I will probably not be able to work on migrating the other plugins for now, but I agree that a feature branch would be a good idea.

[jhnnsrs]

And TemplateNode is back...