Skip to content

Remove SaveSignalHandlingModel#582

Merged
foarsitter merged 1 commit intojazzband:masterfrom
adamchainz:remove_save_signal_handling_model
Nov 6, 2023
Merged

Remove SaveSignalHandlingModel#582
foarsitter merged 1 commit intojazzband:masterfrom
adamchainz:remove_save_signal_handling_model

Conversation

@adamchainz
Copy link
Copy Markdown
Contributor

@adamchainz adamchainz commented Oct 26, 2023

Problem

SaveSignalHandlingModel is out of sync with upstream Django, missing bug fixes and completely broken on Django 5.0. I don’t want to check it against all supported Django versions, and I expect it will go out of sync again.

SaveSignalHandlingModel was added in #285 but hasn't seen any development since. It seems unpopular since there have been no issues reported, and the only results on GitHub code search are from django-model-utils copies: https://github.com/search?q=SaveSignalHandlingModel+language%3APython&type=code&l=Python

Also, the class is probably unnecessary in most cases since bulk_create() bypasses signals, so an easy replacement:

MyModel.objects.bulk_create([MyModel(myfield="thing")])

Bypassing a specific receiver can be done by adding a thread-local flag to those receivers.

Solution

Drop SaveSignalHandlingModel.

Commandments

  • Write PEP8 compliant code.
  • Cover it with tests.
  • 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).

@adamchainz adamchainz force-pushed the remove_save_signal_handling_model branch from cdca114 to 11f3a53 Compare October 26, 2023 15:56
@codecov
Copy link
Copy Markdown

codecov bot commented Oct 26, 2023

Codecov Report

Merging #582 (11f3a53) into master (0ec4ac5) will decrease coverage by 0.04%.
The diff coverage is 100.00%.

@@            Coverage Diff             @@
##           master     #582      +/-   ##
==========================================
- Coverage   95.12%   95.08%   -0.04%     
==========================================
  Files           6        6              
  Lines         820      794      -26     
==========================================
- Hits          780      755      -25     
+ Misses         40       39       -1     
Files Coverage Δ
model_utils/models.py 100.00% <100.00%> (+1.07%) ⬆️

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

@adamchainz adamchainz mentioned this pull request Oct 26, 2023
5 tasks
@foarsitter
Copy link
Copy Markdown
Contributor

I have no objection against dropping SaveSignalHandlingModel but we are breaking peoples code with it. A solution that comes to my mind is keep the SaveSignalHandlingModel model but remove the implementation of save_base. This way we can add a deprecation warning in v4.4 and remove it in v5.

@adamchainz
Copy link
Copy Markdown
Contributor Author

If we remove save_base the class will exist but silently no longer work. I think that is unpreferable compared to a clear ImportError .

@foarsitter foarsitter merged commit 11de64b into jazzband:master Nov 6, 2023
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.

2 participants