fix: same type list by superlevure · Pull Request #1492

fix: same type list by superlevure · Pull Request #1492 · graphql-python/graphene-django

Fix for #1225

Allow to use a recursive type for DjangoListField

(fix suggested by @tcleonard)

Choose a reason for hiding this comment

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

Looks good to me based on the tests, thanks for adding support for this!


		with pytest.raises(AssertionError):
		DjangoListField(TestType)
		with pytest.raises(TypeError):

Choose a reason for hiding this comment

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

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.

Choose a reason for hiding this comment

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

Choose a reason for hiding this comment

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

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

Choose a reason for hiding this comment

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

That's a good point, done in cc45b0b

Choose a reason for hiding this comment

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

LGTM

On a side note I notice a lot of propertys (if not all of them) in DjangoListField and DjangoConnectionField are things that don't change. We could consider using functools.cached_property, but that's for a future issue/PR.

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!

superlevure added a commit to loft-orbital/graphene-django that referenced this pull request

Jan 30, 2024

* fix: same type list

* chore: improve test