Skip to content

Fix enum column conversion by "instantiating" Enum#201

Closed
thejcannon wants to merge 1 commit into
graphql-python:masterfrom
thejcannon:enums_support
Closed

Fix enum column conversion by "instantiating" Enum#201
thejcannon wants to merge 1 commit into
graphql-python:masterfrom
thejcannon:enums_support

Conversation

@thejcannon

Copy link
Copy Markdown

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

@coveralls

Copy link
Copy Markdown

Coverage Status

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

@coveralls

coveralls commented Apr 2, 2019

Copy link
Copy Markdown

Coverage Status

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

@Nabellaleen Nabellaleen added the bug label Apr 3, 2019

@jnak jnak left a comment

Copy link
Copy Markdown
Collaborator

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.

Comment thread graphene_sqlalchemy/tests/test_converter.py Outdated
Comment thread graphene_sqlalchemy/tests/test_converter.py Outdated
Comment thread graphene_sqlalchemy/tests/test_converter.py Outdated
@thejcannon

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

jnak commented Apr 12, 2019

Copy link
Copy Markdown
Collaborator

@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
Copy Markdown
Author

Actually I looked at #165 for this issue and made this: https://github.com/thejcannon/graphene-sqlalchemy/commit/737e288c67fbb875de371ecea23d67e4cdce6156

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

@jnak

jnak commented Apr 12, 2019

Copy link
Copy Markdown
Collaborator

#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

Cito commented Apr 12, 2019

Copy link
Copy Markdown
Member

@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

jnak commented Aug 6, 2019

Copy link
Copy Markdown
Collaborator

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