Skip to content

Conversation

@JoshFerge
Copy link
Contributor

@JoshFerge JoshFerge commented Jan 13, 2025

fixes #718

Edit by @intgr

The background of this change is described in #827. If this change caused a regression in your use case, please comment there ➔ #827

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.

@JoshFerge
Copy link
Contributor Author

I could not find any Sequence usages in https://github.com/encode/django-rest-framework/blob/master/rest_framework/serializers.py

Thanks!

just fixed the type ignore error in ci

@intgr
Copy link
Contributor

intgr commented Jan 13, 2025

instance is a sequence when the serializer is initialized with many=True. Although it truly is annoying to deal with.

There was a plan to fix this with an overloaded Serializer __new__ method, but it requires changes to mypy:

@JoshFerge
Copy link
Contributor Author

got it. thanks for clarifying! will close.

@JoshFerge
Copy link
Contributor Author

JoshFerge commented Jan 21, 2025

Re-opening, as I think that internally the type will never be a sequence, as the new function will return a ListSerializer if a sequence is passed in. I think this is correct.

As written, the serializer is impossible to write a subclass for. cc @intgr @sobolevn

@intgr
Copy link
Contributor

intgr commented Jan 25, 2025

Sorry for the lack of reply. It's a tricky decision between two bad options and I've been pondering this in the background. I'm rather inclined to merge this.

@asottile-sentry
Copy link
Contributor

actually a simpler patch here is to delete the line that's edited (since BaseSerializer already sets instance: _IN | None)

remove instance type declaration in ModelSerializer
@intgr intgr changed the title fix: model serializer instance should not be sequence [Breaking change] Remove work-around for ModelSerializer.instance field for many=True Sep 18, 2025
@intgr
Copy link
Contributor

intgr commented Sep 18, 2025

The background of this change is described in #827. If this change caused a regression in your use case, please comment there ➔ #827

@intgr intgr merged commit eff0fab into typeddjango:master Sep 18, 2025
1 check passed
@intgr
Copy link
Contributor

intgr commented Sep 18, 2025

Sorry for sitting on this PR for so long.

But since this can potentially regress users, I wanted to be sure that the background and justification is properly documented, but kept procrastinating on that.

In any case it's out now, djangorestframework-stubs 3.16.3 released just now.

Release notes: https://github.com/typeddjango/djangorestframework-stubs/releases/tag/3.16.3

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.

Incorrect Type Hint for ModelSerializer class instance attribute

4 participants