search

graphql-python / graphene-sqlalchemy

Graphene SQLAlchemy integration
http://docs.graphene-python.org/projects/sqlalchemy/en/latest/
MIT License
975 stars 225 forks source link

Get default type of hybrid_property from type annotation #333

Closed conao3 closed 2 years ago

conao3 commented 2 years ago

Fix below TODO comment.

# TODO The default type should be dependent on the type of the property propety.

Examines the return type of the function of hybrid_property and converts it to the default graphene type.

conao3 commented 2 years ago 
In modern python, instead of a class, it contains class name

This commit message is wrong. The truth is, 

Under `from __future__ import annotations`, instead of a class, it contains class name
erikwrede commented 2 years ago

I just looked at your PR, I think it's a useful change!

Is there any reason you're selecting the corresponding Graphene Types using a switch/case statement instead of adapting the annotation-based system that is used to register SQLAlchemy types? (@convert_sqlalchemy_type.register(JSONType))

I think creating an annotation for the hybrid properties (e.g. something like @convert_sqlalchemy_type.register_hybrid(Type) ) would allow for more flexibility, and is IMO the preferred option over statically converting the base types, since it would allow users to add their own support for types like Tuples etc until we include them. What do you think?

erikwrede commented 2 years ago

Thanks again for initiating this @conao3!

conao3 commented 2 years ago

Thank you, too!