fix: same type list #1492
fix: same type list #1492
Conversation
superlevure
requested review from
sjdemartini,
kiendang and
jaw9c
and removed request for
sjdemartini
graphene_django/tests/test_fields.py
Outdated
|
|
||
| with pytest.raises(AssertionError): | ||
| DjangoListField(TestType) | ||
| with pytest.raises(TypeError): |
There was a problem hiding this comment.
It looks like in code, this failure condition is still caused by an assert statement. Just curious then why this is manifesting as a TypeError now.
There was a problem hiding this comment.
Good question! The AssertionError is now caught and re-raised as a TypeError by graphql-core here: https://github.com/graphql-python/graphql-core/blob/0c93b8452eed38d4f800c7e71cf6f3f3758cd1c6/src/graphql/type/definition.py#L734
There was a problem hiding this comment.
Nice, thanks! Maybe it's worth adding a final line to the test to validate that the assertion message (about only using DjangoObjectTypes) is present in the raised exception string, since it seems it will be and that would make for a more thorough test
There was a problem hiding this comment.
That's a good point, done in cc45b0b
|
Thanks for the review @kiendang @sjdemartini, I'll let one of you merge as I don't have the authorization |
|
I'm also unable to merge ("Merging is blocked...base branch restricts merging to authorized users"), but looks good to go! |
* fix: same type list * chore: improve test
Fix for #1225
Allow to use a recursive type for
DjangoListField(fix suggested by @tcleonard)