Add proper return value to MockSet delete #184
Conversation
nfantone
force-pushed
the
delete-return
branch
5 times, most recently
from
49fbfd5 to
b2dc8d3
Compare
|
@stefan6419846 You think we could merge this as well, if there's any interest? And also, after/if we do, could we consider releasing a new version of the library? Would be very useful to make recent additions available to everyone. |
django_mock_queries/query.py
Outdated
| for item in matches(*items_to_remove, **attrs): | ||
| self.items.remove(item) | ||
| self.fire(item, self.EVENT_DELETED) | ||
| item_label = item._meta.label if hasattr(item, '_meta') else type(item).__name__ |
There was a problem hiding this comment.
I had to come up with this alternative labelling for, essentially, non-MockModel items because I noticed some unit tests were declaring MockSet out of native values.
Example:
items = [1, 2, 3]
assert MockSet(*items).count() == len(items)
There was a problem hiding this comment.
If I am not mistaken, we do not have a test for the return value in this case? Could you please add one?
There was a problem hiding this comment.
I'll add it, but I'm not too sure of its value. Is this something we should support even?
There was a problem hiding this comment.
Migrating to a label-only approach, id est avoiding the fallback, might be a breaking change if I understand it correctly. But I am unsure about the actual value used here - would None be enough or not?
There was a problem hiding this comment.
might be a breaking change if I understand it correctly
Technically, yes.
But the intended use case is not really documented anywhere. I only came to the realisation that this was a problem after I saw a MockSet of int in the tests. IRL, this is not something Django supports. In fact if you try to create a QuerySet of anything that doesn't have a _meta attribute, it blows up:
from django.db.models import QuerySet
QuerySet(1)
# AttributeError: 'int' object has no attribute '_meta'There was a problem hiding this comment.
Anyway, I've added the relevant unit tests and kept the fallback, just for consistency's sake. This can be addressed later, if needed.
nfantone
force-pushed
the
delete-return
branch
2 times, most recently
from
d7cf759 to
1233813
Compare
nfantone
force-pushed
the
delete-return
branch
from
1233813 to
a86dd99
Compare
This is a continuation of #154 which seems to have gone stale.