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

Add bulk_update_with_history #650

Merged
merged 8 commits into from
Apr 24, 2020
Merged

Conversation

jihoon796
Copy link
Contributor

@jihoon796 jihoon796 commented Apr 23, 2020

I have yet to add tests or documentation. Once a maintainer gives the green light, I'll finish writing all of the documentation, although I will need help writing tests.

Description

Added bulk_update_with_history, using Django 2.2's bulk_update feature.

Related Issue

#528

Motivation and Context

#528

Types of changes

  • Bug fix (non-breaking change which fixes an issue)
  • New feature (non-breaking change which adds functionality)
  • Breaking change (fix or feature that would cause existing functionality to change)

Checklist:

  • I have run the make format command to format my code
  • My change requires a change to the documentation.
  • I have updated the documentation accordingly.
  • I have read the CONTRIBUTING document.
  • I have added tests to cover my changes.
  • I have added my name and/or github handle to AUTHORS.rst
  • I have added my change to CHANGES.rst
  • All new and existing tests passed.

Using Django 2.2's bulk_update feature. This is significantly simpler than bulk_create_with_history because we don't have to worry about the primary key issue.
Copy link
Collaborator

@rossmechanic rossmechanic left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This looks good to me! Thanks for doing this. Want to add docs and tests? I would go off of the tests for bulk_create_with_history for reference on how to test this

simple_history/manager.py Outdated Show resolved Hide resolved
simple_history/manager.py Show resolved Hide resolved
@rossmechanic
Copy link
Collaborator

rossmechanic commented Apr 23, 2020

Also, since the codebase still technically supports Django 1.11, 2.0, and 2.1, just have the tests skip if the django version is any of those. I'm going to remove support for them before the next release.

@jihoon796
Copy link
Contributor Author

@rossmechanic ready for review.

I use django-simple-history frequently, so happy to contribute.

Copy link
Collaborator

@rossmechanic rossmechanic left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@jihoon796 looks good! Just make the changes to the docs. Also please update the CHANGEs.rst file with

Unreleased
------------
- Added `bulk_update_with_history` utility function (gh-650)

Will approve and merge once that's done. Thanks!

@@ -42,8 +42,28 @@ can add `changeReason` on each instance:
>>> Poll.history.get(id=data[0].id).history_change_reason
'reason'

Bulk Updating a Model with History (New)
~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Suggested change
~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~
~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~

docs/common_issues.rst Outdated Show resolved Hide resolved
docs/common_issues.rst Show resolved Hide resolved
@rossmechanic
Copy link
Collaborator

@jihoon796 also, I just looked at the travis build https://travis-ci.org/github/treyhunner/django-simple-history. Looks like you need to run make format as well. Not sure why travis isn't linking with github checks 🤷

@jihoon796
Copy link
Contributor Author

@rossmechanic updated the docs as requested and ran make docs and make format (this time I double-checked).

@rossmechanic
Copy link
Collaborator

https://travis-ci.org/github/treyhunner/django-simple-history/jobs/679104605
@jihoon796 looks like there's a trailing whitespace error

@jihoon796
Copy link
Contributor Author

@rossmechanic got it - fixed the trailing whitespace error, so it should pass all checks now.

When is the next planned release? Would love to use it as soon as possible.

@rossmechanic
Copy link
Collaborator

Cool. Looks like it's passing everything.

I wasn't planning to do a release for another month (and get some changes in before doing a 3.0 release), which is why I said don't worry about backward compatibility. However, if you want to update this PR so that it's still compatible with django versions 1.11, 2.0 and 2.1, then I'd be happy to get this PR and #636 merged and released this weekend. The other options is to pip install this merge commit directly. What do you want to do?

@rossmechanic
Copy link
Collaborator

rossmechanic commented Apr 24, 2020

for reference, to make this backward compatible you just need to do the following within your bulk_update_with_history function:

if django.VERSION < (2, 2, ):
     raise NotImplementedError("bulk_update_with_history is only available on Django versions 2.2 and later")

@codecov-io
Copy link

codecov-io commented Apr 24, 2020

Codecov Report

Merging #650 into master will decrease coverage by 0.62%.
The diff coverage is 50.00%.

Impacted file tree graph

@@            Coverage Diff             @@
##           master     #650      +/-   ##
==========================================
- Coverage   97.70%   97.08%   -0.63%     
==========================================
  Files          17       17              
  Lines         914      925      +11     
  Branches      136      138       +2     
==========================================
+ Hits          893      898       +5     
- Misses          9       15       +6     
  Partials       12       12              
Impacted Files Coverage Δ
simple_history/utils.py 88.67% <25.00%> (-11.33%) ⬇️
simple_history/manager.py 97.29% <100.00%> (+0.11%) ⬆️

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update f47eb3f...9b962e6. Read the comment docs.

@jihoon796
Copy link
Contributor Author

@rossmechanic Would be great to get the release out as early as possible. I've updated this PR (added your snippet) so that it's still compatible with Django versions 1.11, 2.0 and 2.1. Ran the same tests as earlier as well and everything looks good.

Also - thanks for the quick feedback!

Copy link
Collaborator

@rossmechanic rossmechanic left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thanks @jihoon796 ! I think I should update #635 and get it merged, but I'll release as soon as that is done

@rossmechanic rossmechanic merged commit 3f73db3 into jazzband:master Apr 24, 2020
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.

3 participants