Skip to content

Swappable Vote model#85

Open
harikvpy wants to merge 5 commits intoshellfly:masterfrom
harikvpy:master
Open

Swappable Vote model#85
harikvpy wants to merge 5 commits intoshellfly:masterfrom
harikvpy:master

Conversation

@harikvpy
Copy link
Copy Markdown
Contributor

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.

@codecov
Copy link
Copy Markdown

codecov bot commented Aug 19, 2022

Codecov Report

❌ Patch coverage is 86.66667% with 8 lines in your changes missing coverage. Please review.
✅ Project coverage is 90.80%. Comparing base (89a6f59) to head (85e15b4).
⚠️ Report is 5 commits behind head on master.

Files with missing lines Patch % Lines
vote/utils.py 71.42% 4 Missing ⚠️
vote/models.py 70.00% 2 Missing and 1 partial ⚠️
vote/managers.py 75.00% 1 Missing ⚠️
Additional details and impacted files
@@            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     

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.

🚀 New features to boost your workflow:
  • ❄️ Test Analytics: Detect flaky tests, report on failures, and find test suite problems.

@shellfly
Copy link
Copy Markdown
Owner

shellfly commented Aug 20, 2022

@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.

@harikvpy
Copy link
Copy Markdown
Contributor Author

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
Copy link
Copy Markdown
Contributor Author

harikvpy commented Aug 26, 2022

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.

Copy link
Copy Markdown
Owner

@shellfly shellfly left a comment

Choose a reason for hiding this comment

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

thanks for the updating, I have a few comments, PTAL

Comment thread vote/managers.py
self.field_name = field_name

def vote(self, user_id, action):
'''Returns the Vote object on success. None on failure.'''
Copy link
Copy Markdown
Owner

Choose a reason for hiding this comment

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

This is a breaking change for users who are using this API, why do you change this?

Comment thread vote/models.py
swappable_setting = None

UP = 0
DOWN = 1
Copy link
Copy Markdown
Owner

Choose a reason for hiding this comment

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

this is also a breaking change that users' code could be broken after upgrading if they use this variable.

Comment thread tox.ini
django40: Django==4.0
djangorestframework
swapper: swapper==1.3.0
commands = ./runtests_swapped.py
Copy link
Copy Markdown
Owner

Choose a reason for hiding this comment

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

It seems there are many duplications with the current Github CI setup, could we use GitHub action to do this?

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