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.

Sign up for GitHub

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

Jump to bottom

Add "Nested Transactions" section #355

Open
wants to merge 1 commit into
base: master
Choose a base branch
from
+22 −0
Open

Add "Nested Transactions" section #355

Changes from all commits
Commits
Show all changes
1 commit
Select commit
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
22 changes: 22 additions & 0 deletions README.adoc
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -847,6 +847,28 @@ else
end
----

=== Nested Transactions [[nested-transactions]]
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Similar to what I said in the corresponding RuboCop PR, I think we could group this into a section about avoiding private APIs.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thank you for your suggestion.

However, there may be different reasons why private APIs should not be used, so I'm wondering if it's appropriate to group them all in one section.

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I think the section should remain as it is because the focus should be more on the issues that arise with nested transactions rather than the use of private APIs.

@wata727 I’ve been reading your blog, and I was wondering if you could elaborate a bit more on after_commit in this style guide?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I'm thinking about this PR again now, and I'm beginning to think that the section might not be suitable as a "style guide." I believe it’s worth having RuboCop find the problems caused by joinable: false, but it might be endless to list all such problems in the style guide.

@koic Is there a chance we could add a cop without adding it to the style guide, or do you think problems like this are worth mentioning in the style guide?


Use `requires_new: true` if you want to avoid nested transactions unintentionally becoming part of the parent transaction.
`joinable: false` is a private API and should not be used. If used, serious side effects may occur, such as `after_commit` not working properly.

[source,ruby]
----
# bad
ActiveRecord::Base.transaction(requires_new: true, joinable: false) do
ActiveRecord::Base.transaction do
# ...
end
end

# good
ActiveRecord::Base.transaction(requires_new: true) do
ActiveRecord::Base.transaction(requires_new: true) do
# ...
end
end
----

== Models: Active Record Queries [[activerecord-queries]]

=== Avoid Interpolation [[avoid-interpolation]]
Expand Down