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

Refactor transaction handling #690

Merged

Conversation

bmihelac
Copy link
Member

No description provided.

@coveralls
Copy link

Coverage Status

Coverage decreased (-0.1%) to 90.517% when pulling 77323e7 on bmihelac:refactor-transaction-handling into af35835 on django-import-export:master.

@bmihelac bmihelac requested a review from a team December 1, 2017 07:26
@mgrdcm mgrdcm self-requested a review December 4, 2017 13:01
@mgrdcm
Copy link
Contributor

mgrdcm commented Dec 4, 2017

@bmihelac Could you fast-forward this (it's still showing diffs in the CHANGELOG file for instance) please? Also, I see that this is assigned to "django-import-export/maintainers" for code review, but it appears you're the only person in that group? ;)

@mgrdcm mgrdcm mentioned this pull request Dec 4, 2017
1 task
Add `atomic_if_using_transaction` context manager and use it instead of
if/else statements.

Wrap `before_import`, `import_row`, `after_import` calls into
`atomic_if_using_transaction`.

Add tests.

Refs: django-import-export#609, django-import-export#399, django-import-export#610
@bmihelac bmihelac force-pushed the refactor-transaction-handling branch from 77323e7 to dd1c085 Compare December 4, 2017 13:51
@bmihelac
Copy link
Member Author

bmihelac commented Dec 4, 2017

@mgrdcm rebased it to master branch.sorry for misleading review request

@coveralls
Copy link

coveralls commented Dec 4, 2017

Coverage Status

Coverage increased (+0.6%) to 91.255% when pulling dd1c085 on bmihelac:refactor-transaction-handling into 859641a on django-import-export:master.

@mgrdcm
Copy link
Contributor

mgrdcm commented Dec 4, 2017

No worries! I'll try to take a look tonight, though anyone else who can should go ahead!

@mgrdcm
Copy link
Contributor

mgrdcm commented Dec 7, 2017

Not sure when I'm going to be able to do a thorough review of this, might be a few days. @manelclos or @shaggyfrog can you take a look?

@bmihelac bmihelac closed this Dec 8, 2017
@bmihelac bmihelac reopened this Dec 8, 2017
@coveralls
Copy link

coveralls commented Dec 8, 2017

Coverage Status

Coverage increased (+0.6%) to 91.226% when pulling dd1c085 on bmihelac:refactor-transaction-handling into 859641a on django-import-export:master.

@manelclos
Copy link
Contributor

LGTM, tests are passing (including Django 2.0 and master), plus coverage increases 0.6% Whoo-hoo!

@bmihelac bmihelac merged commit 9a32c6e into django-import-export:master Dec 13, 2017
@bmihelac
Copy link
Member Author

merged, thanks for review

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

4 participants