Refactor invalid method declarations #1566
Conversation
| "'queryset' must be supplied as a named parameter", | ||
| category=DeprecationWarning | ||
| ) | ||
| queryset = args[0] |
There was a problem hiding this comment.
Nice, this is very thoughtful! I actually might have some code fixes to do here in my team's stuff.
There was a problem hiding this comment.
Yes, I have tried to make this change backwards compatible but there may be edge cases which I cannot foresee. It is broken as it is so we have to change it.
| @@ -966,10 +966,22 @@ def iter_queryset(self, queryset): | |||
| else: | |||
| yield from queryset.iterator(chunk_size=self.get_chunk_size()) | |||
|
|
|||
| def export(self, queryset=None, *args, **kwargs): | |||
| def export(self, *args, queryset=None, **kwargs): | |||
There was a problem hiding this comment.
I can't help but feel like this is a problem with the interface -- in Java we would probably overload this and have two different export methods I guess, but maybe we should have an export and an export_subset in a future release? I dunno. It just feels wonky.
There was a problem hiding this comment.
Yes good point. There is an additional problem with exports which I hope to address in the near future (see #1548)
|
Thanks for reviewing - as you say we have to rely on tests, and then fix any other issues which come up. |
Problem
Some of the methods are declared incorrectly. A named parameter appears before the *args declaration which is invalid (e.g. #1565).
Solution
Refactor the methods and add deprecation warnings where appropriate.
Where named parameters were declared before
args, I have corrected the method definition, so thatargsis the first argument. If users have overridden these methods, they will need to update, but I have added checks to ensure backwards compatibility. This is to handle cases where the user is passing the arg as the first param.Acceptance Criteria
Tests and documentation included.