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

By default have SMAR use nested transactions #69

Open
wants to merge 1 commit into
base: master
Choose a base branch
from

Conversation

krainboltgreene
Copy link

Okay, so this is a first pass at a PR where we invite the developer to opt-in OR opt-out of nested transactions. I started small to see how the maintainers would react to this.

@krainboltgreene
Copy link
Author

Any thoughts?

@rafaelfranca
Copy link
Contributor

What is the reason to change this default?

@krainboltgreene
Copy link
Author

Whenever I talk to people about SMAR I ask them about transitions and transactions, usually they have two (incorrect) assumptions:

  1. SMAR (before|around|after) is not in a transaction
  2. A transition (inside of another transition) that raises rollback will rollback changes and also rollback the parent transition

Given that I think it would be really valuable for SMAR to both know about nested transactions (obviously I would have to add more to this PR ) but also act like people expect for things like rollbacks and previous_changes.

I'm willing to put a lot of work into this if you give me the green light.

@rafaelfranca
Copy link
Contributor

That seems reasonable. Let's implement this.

@krainboltgreene
Copy link
Author

Awesome, I'll start this weekend with some failing tests and then go from there.

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