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

Add RollbackUnlessCommitted #2921

Closed
wants to merge 1 commit into from
Closed

Conversation

DoubleDi
Copy link

@DoubleDi DoubleDi commented Mar 9, 2020

Make sure these boxes checked before submitting your pull request.

  • Do only one thing
  • No API-breaking changes
  • New code/logic commented & tested

For significant changes like big bug fixes, new features, please open an issue to make an agreement on an implementation design/plan first before starting it.

What did this pull request do?

Hi @jinzhu ! Awesome refactoring with v2 going on.
Added RollbackUnlessCommitted func for transactions. Helps not to write a lot of rollback code on error checking!

Example

tx := db.Begin()
defer tx.RollbackUnlessCommitted()
if somerrhappened {
     // no need for rollback here
     return
}
if othererrhappened {
     // no need for rollback here
     return
}
tx.Commit()

also please keep in mind that you should write commiTTer with double T.

@DoubleDi DoubleDi mentioned this pull request Mar 9, 2020
@jinzhu
Copy link
Member

jinzhu commented Mar 10, 2020

Hello,

Thank you for your PR.

It won't work after adding the RollbackUnlessCommitted method to TxCommiter as database/sql doesn't implement this method.

And I would suggest we don't add API like this but just using method Transaction like this http://gorm.io/docs/transactions.html#Transactions

Could you open another PR to fix the typo, thank you ;)

@jinzhu jinzhu closed this Mar 10, 2020
@DoubleDi
Copy link
Author

DoubleDi commented Mar 10, 2020

Yes, didn't think of that. Thank you @jinzhu
I reviewed the changes and we actually don't need RollbackUnlessCommitted in TxCommitter, only need it in DB. So maybe this PR is still considerable?
Or maybe we can use you callbacks and put beside CommitOrRollback?

@jinzhu
Copy link
Member

jinzhu commented Mar 10, 2020

Have you checked out the Transaction method? I prefer to use it than adding a new API.

@DoubleDi
Copy link
Author

Ok it's pretty handy too. Thanks

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