Fixes instance check in ListSerializer.to_representation (#8726) #8727
Conversation
|
Some developer will use custom |
|
Shouldn't this only need to check the |
Yes, I forgot this. |
| # Dealing with nested relationships, data can be a Manager, | ||
| # so, first get a queryset from the Manager if needed | ||
| iterable = data.all() if isinstance(data, models.Manager) else data | ||
| iterable = data.all() if isinstance(data, models.manager.BaseManager) else data |
There was a problem hiding this comment.
I triggered the CI, can you please pull from main branch again? the bug was not verify able in the test suite, would you mind investigate further to figure out if we can add some relevant test for this?
There was a problem hiding this comment.
I triggered the CI, can you please pull from main branch again? the bug was not verify able in the test suite, would you mind investigate further to figure out if we can add some relevant test for this?
Yes, but it may take a while.
I'm a little hungry. I'm going to cook dinner first. 😋
Is that what you mean? ( My English is not good )
- Append the test case.
- Merge this branch to master in my fork ( I'm not sure about this ).
- Then pull request again (
encode:masterfrom954-ivory:master).
There was a problem hiding this comment.
I meant pull from encode:master to your working branch for this PR 954-ivory:master so that the recently updated versions in CI could reflect here. And some unit tests :)
There was a problem hiding this comment.
thanks for the rebase, now just appending/adding additional test cases would be really great
954-Ivory
force-pushed
the
fix_instance_check
branch
from
4471c05 to
c9b6b02
Compare
|
there are some issues with pre commit, can you check and fix them? you might need to run flake8 locally |
|
all green now! now another round of review before merge |
3 participants
Issue: #8726