django-vote icon indicating copy to clipboard operation
django-vote copied to clipboard

Swappable Vote model

Open harikvpy opened this issue 3 years ago • 4 comments

I had a requirement to add a field to Vote model and I first forked the code as a private app into my project. Then decided to try implementing the same using Django's swappable model infrastructure. This PR incorporates all the changes related to this.

The files affected might seem quite long, but they are mostly because of code re-org. models.py had to be split into base_models.py and models.py and this required touching all the files that imported models.py.

I have updated the README and the tests. The tests now cover swapped model use-case. Essentially the same tests are repeated using a swapped Vote model.

Pls review the code and if you think it's worth it, you may merge it.

harikvpy avatar Aug 19 '22 09:08 harikvpy

Codecov Report

Merging #85 (85e15b4) into master (89a6f59) will decrease coverage by 2.02%. The diff coverage is 86.66%.

@@            Coverage Diff             @@
##           master      #85      +/-   ##
==========================================
- Coverage   92.82%   90.80%   -2.03%     
==========================================
  Files           8        9       +1     
  Lines         223      250      +27     
  Branches       23       27       +4     
==========================================
+ Hits          207      227      +20     
- Misses         13       19       +6     
- Partials        3        4       +1     
Impacted Files Coverage Δ
vote/models.py 84.78% <70.00%> (-8.87%) :arrow_down:
vote/utils.py 77.41% <71.42%> (-6.80%) :arrow_down:
vote/managers.py 92.62% <75.00%> (+0.06%) :arrow_up:
vote/base_models.py 100.00% <100.00%> (ø)
vote/templatetags/vote.py 100.00% <100.00%> (ø)

Help us with your feedback. Take ten seconds to tell us how you rate us. Have a feature suggestion? Share it here.

codecov[bot] avatar Aug 19 '22 09:08 codecov[bot]

@harikvpy Hi, thanks for considering making contribution back to this repo, and I can see the swappable feature could be useful in some cases. However this change introduces a third library swapper as an extra requirement to use django-vote, this might be a noise for users who don't need this feature.

I can accept this change if you could make it an optional feature like the VoteMixin. Or if someday when the swappable model becomes an official Django feature which doesn't require a third-party library, then it'll be great to have this MR in this project.

shellfly avatar Aug 20 '22 13:08 shellfly

Thanks for the quick feedback. What you raise is indeed a valid concern. It should be fairly simple to make the swappable model mechanism an optional feature. I'll have a look and revert this week.

harikvpy avatar Aug 22 '22 07:08 harikvpy

Swapper package is optional now. The test env have been updated to cover both environments -- with swapper and without it using tox. Using tox allows testing the code against all the popular Django versions out there -- 2.2, 3.2 & 4.0, which is nice.

An unintended consequence of this is that code coverage comes down a little as the conditional import code cannot be tested in the without swapper use case.

harikvpy avatar Aug 26 '22 03:08 harikvpy