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

Remove bool return type restriction on Avram::Database#transaction #626

Merged

Conversation

matthewmcgarvey
Copy link
Member

Fixes #416

This allows for code to use transactions like:

AppDatabase.transaction do
  StepOneOperation.run!
  StepTwoOperation.run!
end

This would only have worked previously if StepTwoOperation.run! returned a boolean. Otherwise true would have to be added as the last line because we required the block to return a Bool.

I believe the original type restriction was put in place to match crystal-db's but it was not intention crystal-lang/crystal-db#145

@@ -156,7 +156,7 @@ abstract class Avram::Database
end

# :nodoc:
def transaction : Bool
def transaction
Copy link
Member

Choose a reason for hiding this comment

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

Not too sure about this change. In theory, if it's ok, then we should be able to remove this line and still have things work. I pulled down this PR, and got 3 failures on SaveOperations.

@@ -156,7 +156,7 @@ abstract class Avram::Database
end

# :nodoc:
def transaction : Bool
def transaction
if current_transaction
yield
Copy link
Member

Choose a reason for hiding this comment

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

Now, as a sort of "hack", we can leave the return type as Bool, but then add a true right after this line. The wrap_in_transaction method will already return the bool for that branch. This lets people put whatever in the block, and we still have that return type. The reason I say "hack" though is, i'm not a huge fan of just have a random static value just chillin there, but It would be a step in the right direction, and no different than what we're doing now 🤔

Copy link

@BrucePerens BrucePerens Mar 11, 2021

Choose a reason for hiding this comment

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

Wouldn't it be better if this method was declared def transaction : Nil ? Anything that attempted to use the value would get a complaint. Oh, I guess Nil is falsey.

@matthewmcgarvey matthewmcgarvey force-pushed the matthewmcgarvey/issue416 branch from 982ba14 to 3a98b1a Compare March 10, 2021 22:25
@matthewmcgarvey
Copy link
Member Author

Forgot about this PR but I have updated it to ignore the return value of the yield so that the block isn't forced to return a boolean.

@matthewmcgarvey matthewmcgarvey merged commit 5e29f75 into luckyframework:master Mar 10, 2021
@matthewmcgarvey matthewmcgarvey deleted the matthewmcgarvey/issue416 branch March 10, 2021 22:40
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.

Avram::Database.transaction block return behavior undocumented and may be incorrect
3 participants