-
Notifications
You must be signed in to change notification settings - Fork 215
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
Replace raise with throw to handle context failure #133
base: v4
Are you sure you want to change the base?
Conversation
@@ -8,7 +8,7 @@ module Interactor | |||
# class MyOrganizer | |||
# include Interactor::Organizer | |||
# | |||
# organizer InteractorOne, InteractorTwo | |||
# organize InteractorOne, InteractorTwo |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
👍
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I'm "requesting changes" on this PR mostly because I want us to think about the implementation for longer before I pull the trigger. Great work so far!
lib/interactor/organizer.rb
Outdated
@@ -76,7 +76,8 @@ module InstanceMethods | |||
# Returns nothing. | |||
def call | |||
self.class.organized.each do |interactor| | |||
interactor.call!(context) | |||
throw(:early_return) if context.failure? |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Duplicating context.fail!
logic here doesn't feel right. I don't have a better suggestion yet, though. :)
lib/interactor/context.rb
Outdated
@@ -123,7 +123,7 @@ def failure? | |||
def fail!(context = {}) | |||
context.each { |key, value| modifiable[key.to_sym] = value } | |||
@failure = true | |||
raise Failure, self | |||
throw :early_return |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I have to let this naming sit with me for a little bit.
context.rollback! if context.failure? | ||
rescue | ||
context.rollback! | ||
raise |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I'd like to see if we could preserve how the primary functionality lived in run!
rather than in run
.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
In general, I need to stare at and think about this for a little while. I'm not as familiar with throw
and catch
as I am with raise
and rescue
, so we just need to think through the edge cases.
@@ -76,7 +76,8 @@ module InstanceMethods | |||
# Returns nothing. | |||
def call | |||
self.class.organized.each do |interactor| | |||
interactor.call!(context) | |||
context.signal_early_return! if context.failure? |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I've extracted throwing symbol from Context#fail
to a dedicated method so we can avoid logic duplication.
661413b
to
6c01d9f
Compare
I've resolved conflicts on this branch. |
I agree that these changes would be a great addition. Any updates on considering this? |
Hello there. I used a monkey-patched behavior that is very similar to this one. Being able to succeed early can be very useful (especially in background jobs for instance), I'd love to see this PR land on master. Is there anything blocking its merge, is there anything we/I can do to help? Thanks ❤️ |
Bump: Been a while and checking back in here. Would still love to see this merged. |
Sorry, is there a reason this was closed? The PR above was never merged. |
@hedgesky Sorry for the late ping on a very old pull request. I'm reviewing work and plans started by Steve before he moved on. Do you happen to remember why this change was being made? All of the comments and documentation here focus on what the change is rather than the problem it was looking to solve. |
Hi @gaffneyc. IIRC it was to provide an "early success" capability without using weird exception handling for it. |
The reason why here is to use exceptions in exceptional cases rather than for control flow as used here. See below: In Ruby, exceptions are a way to handle unexpected situations or errors that occur during the execution of a program. They are objects that can be created, raised, and rescued in your code. Using exceptions "exceptionally" means that you should only employ them for truly unexpected situations, rather than using them as a regular part of your program’s control flow. What are Exceptions in Ruby? Why Use Exceptions Correctly? Best Practices for Using Exceptions By understanding the role of exceptions and using them appropriately, you can write cleaner, more efficient, and more reliable Ruby code. |
@hedgesky Thank you for finding that! The rationale makes sense to avoid potential user error when catching exceptions. I'm not as convinced yet about the need for triggering an early success which skips later Interactors as I feel that will make it harder to fully map an Organizer's behavior. My personal take, though there seems to be a want for it, is that an organizer is expected to call all Interactors it organizes unless there is an error. I'm going to reopen this to keep track of it for future 4.0 release. I appreciate that work that's gone into this. It is also early enough in figuring out what the next version is going to look like that I'm not decided on a direction yet. |
Implements #126.
Thrown symbol name
I've decided to pick
:early_return
because we're going to support early success as well as failure.At the earlier stage of implementing this I introduced an additional argument to throw:
But then I realized it's redundant: context itself knows is it successful or failed.
Aborting organized interactors chain execution on failure
Earlier we used
#call!
which raised an error to ensure this; now it's not true. To tackle this problem I've decided to check if context is failed on each step and throw:early_return
if detect one.So we have two issues here:
Docs
I postponed updating internal methods' documentation until we come to an agreement about proposed changes.
I'll be happy to discuss this!
P.S. integration specs pass without changes, thus external behavior didn't change. I think it's a big win 😄