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

Rename depreacated @transaction.commit_on_success to @transaction.atomic #460

Merged
merged 1 commit into from Sep 3, 2014

Conversation

yuvadm
Copy link
Contributor

@yuvadm yuvadm commented Jun 19, 2014

Fixes these warnings:

home/user/.virtualenvs/proj/lib/python2.7/site-packages/django_facebook/utils.py:316: RemovedInDjango18Warning: commit_on_success is deprecated in favor of atomic.

The fix is a simple rename, no functionality should be affected:

atomic allows us to create a block of code within which the atomicity on the database is guaranteed. If the block of code is successfully completed, the changes are committed to the database. If there is an exception, the changes are rolled back.

https://docs.djangoproject.com/en/dev/topics/db/transactions/#django.db.transaction.atomic

@yuvadm
Copy link
Contributor Author

yuvadm commented Sep 3, 2014

@tschellenbach can we please merge this? Django 1.7 is out and anyone using django-facebook is getting deprecation warnings all over the place. This change is trivial and should pass all existing tests.

@tschellenbach
Copy link
Owner

sorry guys, things are very busy at the moment.
thanks for helping out! Open source is great (sometimes) :)

tschellenbach added a commit that referenced this pull request Sep 3, 2014
Rename depreacated @transaction.commit_on_success to @transaction.atomic
@tschellenbach tschellenbach merged commit 1e80ec3 into tschellenbach:master Sep 3, 2014
@yuvadm yuvadm deleted the atomic branch September 3, 2014 12:55
@yuvadm
Copy link
Contributor Author

yuvadm commented Sep 3, 2014

@tschellenbach no problem, thanks for the quick merge!

@johnshiver
Copy link

Is this change compatible with Django 1.5? It looks like 1.5 doesnt support @transaction.atomic

@yuvadm
Copy link
Contributor Author

yuvadm commented Dec 23, 2014

@johnshiver This is a non-backwards-compatible change, but Django 1.5 is not supported anymore. You can argue that since 1.4 is an LTS version it might need to be supported, but I'm not sure about @tschellenbach's policy on that, you might just need to use an old version of django-facebook as well.

@johnshiver
Copy link

@yuvadm Thanks for the reply! Yeah I will be using 5.3.1 for the time being, just wanted to comment since the docs say the minimum required django version is 1.5, whereas with this change perhaps it should say 1.6

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.

None yet

3 participants