-
Notifications
You must be signed in to change notification settings - Fork 216
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
Fix races that lead to duplicate votes creation #211
base: master
Are you sure you want to change the base?
Conversation
Ok I think this one is next! I'm excited to get your fix published. Want to rebase it for specs? |
7f88df8
to
e6391b0
Compare
Rebased. |
Useful change! @ryanto Any plans to get this in? |
Heya! Yes, I want to get this in. This approach is great and it's what we will be using. If you want to use a fork of this PR I'd say go for it! This PR is a big change, and these sorts of change always end up generating a ton of support issues from folks that don't read the upgrade instructions. There's a tradeoff I need to balance: not supporting concurrency vs all the people that will ignore the upgrade guides, break their acts_as_votable installs once this is merged, and then yell at me :). I think the best way to deal with this is to put this PR in its own big release. Here's my currently plan.
This will help make it clear that upgrading from 1.0 to 2.0 has breaking changes and users need to follow the upgrade instructions. Sorry it has taken so long to get this merged. No good excuse... will happen soon(ish). |
any update on this? |
Any updates here? |
Closes #181
Closes #127
I have also modernized a bit migration file style, validations and added a changelog file (style is adapted from
rubocop
).The most complicated part of this would be for existing users upgrade their apps. I have added a detailed enough description to the changelog, but it would be nice for someone to try that on a production app and see how that goes.