Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
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
Improve auto creation of Graphene Enums. #98
Improve auto creation of Graphene Enums. #98
Changes from 7 commits
67a621f
9dd34ca
2f9145c
3143397
8819829
c2c4a77
c278a6f
ec6d4a2
67d789d
082169e
361b44c
File filter
Filter by extension
Conversations
Jump to
There are no files selected for viewing
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Why this rename? You are still converting a column here.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The functions must have specific names, because columns with ChoiceType and Enum both convert to Enum. The function name would then be
convert_column_to_enum
for both. Actually the other functions should also have more specific names, i.e. contain not only the target type, but also the source type.There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
What are the legitimate cases where one would want to disable duplicate registration?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The
get_type_for_model()
method (used byconvert_sqlalchemy_relationship()
) assumes that there is only one graphene type per model. You may want to make sure that this assumption is met and you haven't accidentally associated two different types with one model.There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This case makes sense to me. I was asking about the legitimate cases where you one does not want this function to raise when there are duplicates.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Ah, ok - good question. Not checking was the behavior until now, and
test_types
would break because CustomCharacter uses the same model as Character. It only matters if you have relationships. Suggestion: Maybe we should always allow duplicate registration, but memorize all of them instead of only the last. And only when get_type_for_model is called, we throw an error which lists the duplicates. I.e. you only get an error, when this is really a problem.There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Why does
register_enum
takes a name instead ofSQLAlchemyEnumType
? It seems inconsistent withregister
andregister_sort_params
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Because not all enums are derived from SQLAlchemyEnumTypes. Some are created for sorting purposes, they don't exist as SQLAlchemyEnumTypes. All of the enums should use the same registry because Enum names in the schema must be unique - you want to get an error when a sort enum accidentally has the same name as an existing enum for a column, and you want to have a way for requesting registered enums by name.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I see. How about we instead have this function take an enum or a column that maps to SQLAlchemyEnumType to keep all that logic encapsulated here? In both cases, we would be able to extract a name.
Regarding name collisions, the names need to be unique across all types (vs just within Enums).
graphene
already raises an error to prevent this. Why do we need to handle this ingraphene-sqlalchemy
as well?There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Right. I will think about the whole mechanism again and try to create a better-thought-out solution. But maybe I'll do this as a new PR.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Hmm in theory all columns should have a name. I wonder if this can only happen in our tests because SQLAlchemy is not fully initialized.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This is the name of the SQLAlchemy Enum type, not the name of the column. That name may not exist.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I see. What are the benefits of having this function take a
SQLAlchemyEnumType
instead of a column that maps to aSQLAlchemyEnumType
? I can't think of a case where we would have aSQLAlchemyEnumType
without its associated column.There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
You would need to get the SQLAlchemy enum type anyway with
column.type
. Theconvert_sqlalchemy_column
function already does this. We need the name of the enum type, not the name of the column, because the same enum could be used in different columns.There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Why does
get_type_for_enum
need to register the enum? It seems inconsistent withget_type_for_model
andget_sort_params_for_model
?There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The naming of the methods and division between registry and utils modules could certainly be improved. Need to sleep over it, or we refactor this in a new PR.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I'd prefer to address this in this PR because this may lead to unexpected behaviors. For example, as is we are getting different enums for the same un-named sql enum:
Ideally a getter should not have any side effects. Why can't we just return the enum from the registry?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
See below, I'll try to come up with a better solution.