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

Formset get_queryset() returns QuerySet #2174

Merged
merged 3 commits into from
May 27, 2024

Conversation

MrkGrgsn
Copy link
Contributor

I have made things!

Set the return type of BaseModelFormset.get_queryset() to QuerySet[_M].

In previous version, it has a return type of _IndexableCollection but QuerySet is no longer a type of Collection (#2095) and in Django main branch, 5.0 and 4.2 this function returns a plain old QuerySet.

Copy link
Member

@sobolevn sobolevn left a comment

Choose a reason for hiding this comment

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

Technically you can return other data types from get_queryset, see https://github.com/django/django/blob/main/django/forms/models.py

How can we solve this differently?

@MrkGrgsn
Copy link
Contributor Author

MrkGrgsn commented May 21, 2024

Thanks @sobolevn. You're right, QuerySet is insufficient. As far as I can see, uses of get_queryset rely on __iter__ , __getitem__, and __len__ and I couldn't see a type providing that so I created a new abstract collection type. I struggled with finding a generic name that wasn't too long (IndexableSizedIterable?) so went for _QuerySetLike. Let me know what you'd prefer.

@sobolevn sobolevn requested review from intgr and flaeppe May 22, 2024 08:56
@intgr
Copy link
Collaborator

intgr commented May 22, 2024

Technically you can return other data types from get_queryset, see https://github.com/django/django/blob/main/django/forms/models.py

That's surprising. What exactly is this link pointing to? I just found one get_queryset method there which does return a QuerySet.

@flaeppe
Copy link
Member

flaeppe commented May 22, 2024

I think it's totally fine to have the method named get_queryset typed to return a QuerySet.

And if there's any user(s) that override the method to change the return type to e.g. list[MyModel] they can just # type: ignore[...].

But if I'm using the default implementation of self.get_queryset() without having done any overrides I would very much like to be able to expect a QuerySet[...] in order to be able to do e.g. self.get_queryset().filter(...) without any errors.

@sobolevn
Copy link
Member

This is also fine 👍

@MrkGrgsn
Copy link
Contributor Author

If I've followed the thumbsup's correctly, the preferred solution is QuerySet. Let me know if I'm wrong otherwise I'll revert the 2nd commit tomorrow. 👍

@intgr
Copy link
Collaborator

intgr commented May 23, 2024

Yes, I think QuerySet[_M] is more appropriate here.

@flaeppe
Copy link
Member

flaeppe commented May 24, 2024

If I've followed the thumbsup's correctly, the preferred solution is QuerySet. Let me know if I'm wrong otherwise I'll revert the 2nd commit tomorrow. 👍

You're right in that the preferred solution is QuerySet[_M] so I think it's enough to drop 8ee0e48

@MrkGrgsn
Copy link
Contributor Author

Sorry for the delay, I've reverted the 2nd commit.

@flaeppe
Copy link
Member

flaeppe commented May 27, 2024

Thank you

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Development

Successfully merging this pull request may close these issues.

4 participants