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

Remove arguments from InheritanceQuerySetMixin._clone() #567

Merged
merged 1 commit into from
Mar 21, 2024

Conversation

mthuurne
Copy link
Contributor

@mthuurne mthuurne commented Apr 19, 2023

Problem

InheritanceQuerySetMixin._clone() accepts and then ignores a klass and setup argument, plus any keyword arguments. The QuerySet._clone() method has no arguments in either Django 3.2 or 4.1 and when using a static code checker like mypy, this is reported as incompatible base classes for InheritanceManager.

Solution

I'm assuming the arguments were necessary for a version of Django that is no longer supported by django-model-utils. Therefore I removed the extra arguments.

Commandments

  • Write PEP8 compliant code.
  • Cover it with tests. (The _clone() method was already covered.)
  • Update CHANGES.rst file to describe the changes, and quote according issue with GH-<issue_number>.
  • Pay attention to backward compatibility, or if it breaks it, explain why.
  • Update documentation (if relevant).

I chose to not add an entry in CHANGES.rst, as this is a cleanup that should not be noticeable to users.

The `QuerySet._clone()` method has no arguments in either Django 3.2
or 4.1, so I'm assuming the arguments were necessary for a version
of Django that is no longer supported by `django-model-utils`.
@codecov
Copy link

codecov bot commented Apr 19, 2023

Codecov Report

Merging #567 (033fca4) into master (6154436) will not change coverage.
The diff coverage is 100.00%.

@@           Coverage Diff           @@
##           master     #567   +/-   ##
=======================================
  Coverage   95.12%   95.12%           
=======================================
  Files           6        6           
  Lines         820      820           
=======================================
  Hits          780      780           
  Misses         40       40           
Impacted Files Coverage Δ
model_utils/managers.py 98.08% <100.00%> (ø)

📣 We’re building smart automated test selection to slash your CI/CD build times. Learn more

@mthuurne mthuurne mentioned this pull request Mar 14, 2024
@foarsitter
Copy link
Contributor

This PR will break backwards compatibility right?

Is it an idea to just accept **kwargs and emit a DeprecationWarning when kwargs > 0?

@mthuurne
Copy link
Contributor Author

This PR will break backwards compatibility right?

It doesn't break any existing functionality, but instead of any passed arguments being silently ignored, a TypeError will be raised. So it does have the potential to escalate pre-existing problems in calling code.

However, as _clone() is not a public method, the only caller would be QuerySet or a subclass of it. So the typical application code will not be calling it directly and Django 3.2 always calls it without arguments.

Is it an idea to just accept **kwargs and emit a DeprecationWarning when kwargs > 0?

Yes, but if we want to be that careful, probably it should catch both *args and **kwargs, because calling code might use positional arguments instead.

I'll leave the decision up to you: personally, I don't think it's likely this change will break any existing code, but if you prefer to be extra cautious, I'll make it accept *args and **kwargs and add the DeprecationWarning.

@foarsitter foarsitter added this to the 4.5.0 milestone Mar 21, 2024
@foarsitter
Copy link
Contributor

Extra cautiousness isn't necessary since your argument about the private nature of the method is a good one.

@foarsitter foarsitter merged commit 89b7662 into jazzband:master Mar 21, 2024
8 checks passed
@mthuurne mthuurne deleted the remove-clone-args branch March 22, 2024 12:58
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

2 participants