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

Fix enum column conversion by "instantiating" Enum #201

Closed
wants to merge 1 commit into from

Conversation

thejcannon
Copy link

Fixes #196 and #199.
See also graphql-python/graphene#932

@coveralls
Copy link

Coverage Status

Coverage increased (+0.09%) to 91.989% when pulling d7f9085 on thejcannon:enums_support into c9af40c on graphql-python:master.

@coveralls
Copy link

coveralls commented Apr 2, 2019

Coverage Status

Coverage increased (+0.09%) to 91.989% when pulling 53f107e on thejcannon:enums_support into c9af40c on graphql-python:master.

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.

Looks good! I just had a couple of very minor comments.

graphene_sqlalchemy/tests/test_converter.py Outdated Show resolved Hide resolved
graphene_sqlalchemy/tests/test_converter.py Outdated Show resolved Hide resolved
graphene_sqlalchemy/tests/test_converter.py Outdated Show resolved Hide resolved
@thejcannon
Copy link
Author

Once this gets merged in, I wouldn't mind resolving the conflicts in #165 with my changes.
I'll post a new PR for that.

@jnak
Copy link
Collaborator

jnak commented Apr 12, 2019

@thejcannon After commented on this PR, I caught up with this one #98. Do you expect your PR to conflict with that one as well?

@thejcannon
Copy link
Author

Actually I looked at #165 for this issue and made this: thejcannon@737e288

So you could close both of those once that PR is made/merged.

@jnak
Copy link
Collaborator

jnak commented Apr 12, 2019

#165 seems to duplicate most of the thing from #98. Since #98 was first and there has been quite a bit of conversation there, I'd rather merge this one over #165.

Would you mind taking a look at it and let me know what you think?

@Cito
Copy link
Member

Cito commented Apr 12, 2019

@jnak - #165 is pretty much like #98, but only part of it. It does not contain the name mangling, does not integrate the sort enums, and has fewer tests. It's a bit unfortunate that this stalled for so long that people created duplicate solutions and the code diverged.

@jnak
Copy link
Collaborator

jnak commented Aug 6, 2019

Closing since #210 got merged

@jnak jnak closed this Aug 6, 2019
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Enum ChoiceType column results in error
5 participants