Skip to content

add support for enums in sqlalchemy ChoiceType #240

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

Merged
merged 5 commits into from
Sep 11, 2019
Merged

add support for enums in sqlalchemy ChoiceType #240

merged 5 commits into from
Sep 11, 2019

Conversation

metheoryt
Copy link
Contributor

Hi! I've found that I cannot use python enums with sqlalchemy_utils's ChoiceType, so here's my solution for it

@coveralls
Copy link

coveralls commented Aug 5, 2019

Coverage Status

Coverage increased (+0.02%) to 97.343% when pulling 640cd17 on metheoryt:patch-1 into 8ea2086 on graphql-python:master.

@coveralls
Copy link

Coverage Status

Coverage decreased (-0.2%) to 97.104% when pulling 5117e53 on metheoryt:patch-1 into c89cf80 on graphql-python:master.

@jnak
Copy link
Collaborator

jnak commented Aug 6, 2019

Thanks for sending a PR. Would you mind updating the tests in test_converter.py so both cases are tested?

You can see how to run the tests locally here: https://github.com/graphql-python/graphene-sqlalchemy/blob/master/CONTRIBUTING.md

Copy link
Collaborator

@jnak jnak left a comment

Choose a reason for hiding this comment

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

Oops - I didn't see your update until now. Thanks for adding the tests. I have one minor comment about a deleted TODO but I'm fine punting on it if you don't have time to put it back.

Since it seems you are using ChoiceType, you may be interested in addressing the TODO in a later PR. It should make it a lot easier to deal with auto-generated Enums (see my comment).

Copy link
Collaborator

@jnak jnak left a comment

Choose a reason for hiding this comment

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

Oops - completely fell off my radar. Sorry for the delay.

@jnak jnak merged commit 0544f81 into graphql-python:master Sep 11, 2019
@jnak
Copy link
Collaborator

jnak commented Sep 11, 2019

@metheoryt Btw in case you're interested, here is more context on the TODO. The enums auto-generated by ChoiceLabel are not as clean and robust as other enum columns:

  • the name of the enum include the the table and the column name
  • the name of the enum cannot be customized
  • a duplicated enum is created (with a different name) if the ChoiceType is shared across multiple columns

This PR added proper support for enums but did not include ChoiceType. I think it would be quite straightforward to have ChoiceType leverage the same infrastructure.

Cheers

@metheoryt metheoryt deleted the patch-1 branch May 4, 2020 12:43
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