Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Get default type of hybrid_property from type annotation #333

Closed
wants to merge 4 commits into from

Conversation

conao3
Copy link

@conao3 conao3 commented Mar 4, 2022

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
Copy link
Author

conao3 commented Mar 5, 2022

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
Copy link
Member

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?

return type_ in (args + [x.__name__ for x in args])


def convert_hybrid_type(type_, column):
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

See my comment in the PR

Copy link
Contributor

@flipbit03 flipbit03 Apr 28, 2022

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I need this feature, so I picked it up to try to finish the work.

Just as a heads up, I tried to use the actual @singledispatch mechanism and it is not a very good fit, since we are using actual types from a type annotation, and not values. So the types inside a type annotation usually descend from the base type, so @singledispatch fails to match anything in a meaningful way 😢

I'm trying to code an approach similar to that where we can decorate functions and switch based on value or on some kind of matcher function, so it will basically become a glorified (but as you said, customizable by the end user in a very flexible way) switch statement. I foresee we will still need some kind of escape hatch using the 'default' (no match) function to analyze deeper types like List[T] and things like that.

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Let me know what you come up with! 😃

Copy link
Contributor

@flipbit03 flipbit03 Apr 28, 2022

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I will, thank you 🙏 I'll open a new Pull Request for #333 from my own fork as soon as I have the basics working.

Copy link
Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I want to finish this work. I will work on this PR tomorrow, so can it wait?

Copy link
Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I am branching by case statement because it returning the corresponding Graphene type in a class of type type obtained by __annotations__.
functools.singledispatch is a way to achieve polymorphism by instance type, but I don't know at this time how to achieve polymorphism by the class itself.

Copy link
Contributor

@flipbit03 flipbit03 Apr 29, 2022

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Indeed, the standard library's singledispatch approach is inadequate for what we want. I provided a solution using a different type of matching but leaving the comfort/developer a @singledispatch-like experience provides.

I finished the work I intended and opened a pull request, mentioning this issue in it, looking forward for the review.

@erikwrede
Copy link
Member

Thanks again for initiating this @conao3!

@conao3 conao3 deleted the hybrid-property-type branch April 30, 2022 00:28
@conao3
Copy link
Author

conao3 commented Apr 30, 2022

Thank you, too!

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants