Allow developers to define #call with arguments for convenience #135

laserlemon · 2017-03-31T18:02:05Z

If the call instance method accepts arguments, those arguments will be automatically assigned from the provided context, matching on name. This works for both positional and keyword arguments. If an argument is specified but no matching value is provided in the context, an ArgumentError is raised.

If the "call" instance method accepts arguments, those arguments will be automatically assigned from the provided context, matching on name. This works for both positional and keyword arguments. If an argument is specified but no matching value is provided in the context, an ArgumentError is raised.

Required keyword arguments appeared in Ruby 2.1, after the introduction of keyword arguments in Ruby 2.0.

This also improves performance of the Interactor#arguments_for_call method by not duplicating the table of data held internally by the context. As a happy side effect, this adds the Interactor::Context#include? method which may be helpful for developers.

…by not interacting directly with OpenStruct#modifiable.

laserlemon · 2017-03-31T20:53:51Z

@hedgesky Would you mind taking a look at this? Thank you!

hedgesky · 2017-03-31T20:58:19Z

Yeah, I'm watching the repo 😄
Feels a bit like magic; however, underlying code is simple and straightforward, great!
Excellent job! 💪

hedgesky · 2017-03-31T21:00:04Z

lib/interactor/context.rb

@@ -121,7 +121,7 @@ def failure?
    #
    # Raises Interactor::Failure initialized with the Interactor::Context.
    def fail!(context = {})
-      context.each { |key, value| modifiable[key.to_sym] = value }
+      context.each { |key, value| self[key] = value }


hedgesky · 2017-03-31T21:01:03Z

lib/interactor.rb

+  # based on their names.
+  #
+  # Returns an Array of arguments to be applied as an argument list.
+  def arguments_for_call # rubocop:disable Metrics/MethodLength


Really cool implementation!

I think I'll end up backing out the positional argument support. It feels a little too magical in hindsight. Plus, I think there's a bug that I haven't tested for yet that could misplace positional arguments. The case I'm wondering about is:

class MyInteractor include Interactor def call(foo, hello = "world") context.output = [foo, hello] end end result = MyInteractor.call(hello: "Anton") result.output # This should raise an ArgumentError, but might be ["Anton", "world"]

The only thing that bothers me is that supporting only keywords arguments may be confusing. #call will not act like a standard Ruby method, but will have its own rules instead. There would be additional lines in Readme, things to keep in memory, things to explain to a newcomer into a project-which-uses-interactors, etc.

I thought about alternative syntax:

class MyInteractor include Interactor required_arguments :user, :order optional_arguments :email, send_notification: true def call end end

On the one hand, keyword arguments do the same and in more expressive way and they are already familiar.
On the other hand, this familiarity may become a bad thing in our case. E.g, if I can write:

def call(hello:) end MyInteractor.call(hello: "Steve")

why couldn't I write

def call(name) end MyInteractor.call("Steve")

? And here magic falls apart.

I removed positional argument support. I think it just muddied the waters and could be achieved just as well with keyword arguments.

As for the last two code examples in your comment above, I don't feel like the arguments accepted by the call class method needs to necessarily agree 100% with the arguments accepted by the call instance method, but scrapping positional argument support certainly does make it more consistent between the two, which I like. What are your thoughts, @hedgesky?

Personally, I'd rather raise an exception for positional arguments. It would help avoiding such situations (just checked this in call-with-keywords branch):

class A include Interactor def call(foo, bar: "") puts "Foo is #{foo}" puts "Bar is #{bar}" end end A.call(bar: 1) # Foo is {:bar=>1} # Bar is

Also this would draw a clear line between approaches we support and ones we don't.
@laserlemon, I'm open to discussion.

Yeah, great catch! Maybe when we include Interactor, we can check the call instance method's argument signature to make sure it's either all keywords or nothing at all. That way we can raise an error at load time rather than at run time. Thoughts?

You are right, aborting execution on load would be perfect. But I guess it's possible only if we force users to include Interactor after defining call, which is inconvenient:

module M def self.included(base) print "From #{base.name}: " puts base.instance_methods.grep(/call/).inspect end end class A include M def call(foo) end end class B def call(foo) end include M end # From A: [] # From B: [:call]

However, with runtime approach, method with wrong signature will raise an error at its very first run. I hope even those who don't write automated tests check their code manually at least once before deploying to production 😃

There were edge cases that weren't yet covered by the specs that would fail for combinations of positional and keyword arguments depending on what's available in the context. For example: class MyInteractor include Interactor def call(a, b: "bears") context.output = [a, b] end end MyInteractor.call(b: "beets").output # => [{ b: "beets" }, "bears"] Plus, this simplifies the interface by giving the developer one choice rather than multiple competing choices that achieve the same thing.

olleolleolle · 2017-05-30T07:15:52Z

interactor.gemspec

@@ -17,7 +17,7 @@ Gem::Specification.new do |spec|
  spec.files      = `git ls-files`.split($INPUT_RECORD_SEPARATOR)
  spec.test_files = spec.files.grep(/^spec/)

-  spec.required_ruby_version = ">= 2.0"
+  spec.required_ruby_version = ">= 2.1"


A big thank-you for maintaining this value.

bogdanRada · 2017-11-13T09:30:45Z

lib/interactor.rb

+    positional_arguments = []
+    keyword_arguments = {}
+
+    method(:call).parameters.each do |(type, name)|


i think should be here: self.class.instance_method(:call) instead of method(:call), because for some reason method(:call) returns the parameters from the class method "call", and not from the instance method "call" . I know this because i actually tried this and the only way i could make it work was to use self.class.instance_method .
Or maybe i am missing something.

elsurudo · 2018-07-29T18:13:17Z

@laserlemon This seems like a nice improvement, but an issue yet to be fixed as per @bogdanRada's comment?

keithmattix · 2018-09-21T18:35:23Z

Any updates on this?

edisonywh · 2019-01-24T11:25:39Z

Pinging it again. Any updates on this?

kunalbhagawati · 2019-07-08T12:05:11Z

Pinging it again, again. :)
Any updates on this PR?

nitsujri · 2019-08-01T03:58:06Z

+1 for Custom DSL instead of positional arguments or even keyword-only .call.

.call is special and if it operates similar to a normal ruby function but not exactly like it, it will create lots of headaches.

#140 for reference.

laserlemon added the feature label Mar 31, 2017

laserlemon added this to the v4.0.0 milestone Mar 31, 2017

laserlemon mentioned this pull request Mar 31, 2017

Change Interactor#call to receive a partial context via keyword arguments #123

Closed

laserlemon added 4 commits March 31, 2017 16:52

Target Ruby versions >= 2.1

822c2a4

Required keyword arguments appeared in Ruby 2.1, after the introduction of keyword arguments in Ruby 2.0.

Address Rubocop concerns

cf6b273

This also improves performance of the Interactor#arguments_for_call method by not duplicating the table of data held internally by the context. As a happy side effect, this adds the Interactor::Context#include? method which may be helpful for developers.

Refactor Interactor::Context#fail! to be simpler

a0b9688

…by not interacting directly with OpenStruct#modifiable.

laserlemon force-pushed the call-with-keywords branch from 67824df to a0b9688 Compare March 31, 2017 20:52

hedgesky reviewed Mar 31, 2017

View reviewed changes

hedgesky approved these changes Mar 31, 2017

View reviewed changes

laserlemon added the WIP label Apr 1, 2017

laserlemon removed the WIP label Apr 28, 2017

olleolleolle reviewed May 30, 2017

View reviewed changes

bogdanRada reviewed Nov 13, 2017

View reviewed changes

frankieroberto mentioned this pull request Jun 11, 2020

Refactor update owner audit activity OfficeForProductSafetyAndStandards/product-safety-database#700

Merged

2 tasks

This was referenced Jul 12, 2020

Allow developers to define #call with arguments for convenience taylorthurlow/interactor#1

Closed

Allow developers to define #call with arguments for convenience taylorthurlow/interactor#2

Open

laserlemon closed this Apr 19, 2024

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Allow developers to define #call with arguments for convenience #135

Allow developers to define #call with arguments for convenience #135

laserlemon commented Mar 31, 2017

laserlemon commented Mar 31, 2017

hedgesky commented Mar 31, 2017

hedgesky Mar 31, 2017

hedgesky Mar 31, 2017

laserlemon Apr 1, 2017

hedgesky Apr 2, 2017 •

edited

Loading

laserlemon Apr 28, 2017

hedgesky Apr 29, 2017

laserlemon May 1, 2017

hedgesky May 1, 2017

olleolleolle May 30, 2017

bogdanRada Nov 13, 2017

elsurudo commented Jul 29, 2018

keithmattix commented Sep 21, 2018

edisonywh commented Jan 24, 2019

kunalbhagawati commented Jul 8, 2019

nitsujri commented Aug 1, 2019

Allow developers to define #call with arguments for convenience #135

Allow developers to define #call with arguments for convenience #135

Conversation

laserlemon commented Mar 31, 2017

laserlemon commented Mar 31, 2017

hedgesky commented Mar 31, 2017

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

hedgesky Apr 2, 2017 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

elsurudo commented Jul 29, 2018

keithmattix commented Sep 21, 2018

edisonywh commented Jan 24, 2019

kunalbhagawati commented Jul 8, 2019

nitsujri commented Aug 1, 2019

hedgesky Apr 2, 2017 •

edited

Loading