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

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

Closed
wants to merge 5 commits into from
Closed
Show file tree
Hide file tree
Changes from 4 commits
Commits
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
4 changes: 3 additions & 1 deletion .rubocop.yml
Original file line number Diff line number Diff line change
@@ -1,7 +1,7 @@
# This should always correspond to the required Ruby version specified in the
# gemspec.
AllCops:
TargetRubyVersion: 2.0
TargetRubyVersion: 2.1

# TODO: What should we do here?
Style/FrozenStringLiteralComment:
Expand Down Expand Up @@ -43,3 +43,5 @@ Style/StringLiterals:
EnforcedStyle: double_quotes
Style/SymbolArray:
Enabled: false
Style/WordArray:
Enabled: false
2 changes: 0 additions & 2 deletions .travis.yml
Original file line number Diff line number Diff line change
Expand Up @@ -8,15 +8,13 @@ cache: bundler
language: ruby
matrix:
allow_failures:
- rvm: "2.0"
- rvm: ruby-head
notifications:
webhooks:
on_start: always
urls:
- http://buildlight.collectiveidea.com/
rvm:
- "2.0"
- "2.1"
- "2.2"
- "2.3.4"
Expand Down
2 changes: 1 addition & 1 deletion interactor.gemspec
Original file line number Diff line number Diff line change
Expand Up @@ -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"
Copy link
Contributor

Choose a reason for hiding this comment

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

A big thank-you for maintaining this value.


spec.add_development_dependency "bundler", "~> 1.9"
spec.add_development_dependency "rake", "~> 10.4"
Expand Down
28 changes: 27 additions & 1 deletion lib/interactor.rb
Original file line number Diff line number Diff line change
Expand Up @@ -140,7 +140,7 @@ def run
# Raises Interactor::Failure if the context is failed.
def run!
with_hooks do
call
call(*arguments_for_call)
context.called!(self)
end
rescue
Expand All @@ -163,4 +163,30 @@ def call
# Returns nothing.
def rollback
end

private

# Internal: Determine what arguments (if any) should be passed to the "call"
# instance method when invoking an Interactor. The "call" instance method may
# accept any combination of positional and keyword arguments. This method
# will extract values from the context in order to populate those arguments
# based on their names.
#
# Returns an Array of arguments to be applied as an argument list.
def arguments_for_call # rubocop:disable Metrics/MethodLength
Copy link
Contributor

Choose a reason for hiding this comment

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

Really cool implementation!

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

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"]

Copy link
Contributor

@hedgesky hedgesky Apr 2, 2017

Choose a reason for hiding this comment

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

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.

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

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?

Copy link
Contributor

Choose a reason for hiding this comment

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

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.

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

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?

Copy link
Contributor

Choose a reason for hiding this comment

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

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 😃

positional_arguments = []
keyword_arguments = {}

method(:call).parameters.each do |(type, name)|

Choose a reason for hiding this comment

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

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.

next unless context.include?(name)

case type
when :req, :opt then positional_arguments << context[name]
when :keyreq, :key then keyword_arguments[name] = context[name]
end
end

positional_arguments << keyword_arguments if keyword_arguments.any?
positional_arguments
end
end
11 changes: 10 additions & 1 deletion lib/interactor/context.rb
Original file line number Diff line number Diff line change
Expand Up @@ -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 }
Copy link
Contributor

Choose a reason for hiding this comment

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

👍

@failure = true
raise Failure, self
end
Expand Down Expand Up @@ -158,6 +158,15 @@ def rollback!
@rolled_back = true
end

# Public: Check for the presence of a given key in the context. This does
# not check whether the value is truthy, just whether the key is set to any
# value at all.
#
# Returns true if the key is found or false otherwise.
def include?(key)
table.include?(key.to_sym)
end

# Internal: An Array of successfully called Interactor instances invoked
# against this Interactor::Context instance.
#
Expand Down
20 changes: 20 additions & 0 deletions spec/interactor/context_spec.rb
Original file line number Diff line number Diff line change
Expand Up @@ -164,6 +164,26 @@ module Interactor
end
end

describe "#include?" do
it "returns true if the key is found" do
context = Context.build(foo: "bar")

expect(context.include?(:foo)).to eq(true)
end

it "returns true if the symbolized key is found" do
context = Context.build(foo: "bar")

expect(context.include?("foo")).to eq(true)
end

it "returns false if the key is not found" do
context = Context.build(foo: "bar")

expect(context.include?(:hello)).to eq(false)
end
end

describe "#_called" do
let(:context) { Context.build }

Expand Down
210 changes: 210 additions & 0 deletions spec/interactor_spec.rb
Original file line number Diff line number Diff line change
@@ -1,3 +1,213 @@
describe Interactor do
include_examples :lint

describe "#call" do
let(:interactor) { Class.new.send(:include, described_class) }

context "positional arguments" do
it "accepts required positional arguments" do
interactor.class_eval do
def call(foo)
context.output = foo
end
end

result = interactor.call(foo: "baz", hello: "world")

expect(result.output).to eq("baz")
end

it "accepts optional positional arguments" do
interactor.class_eval do
def call(foo = "bar")
context.output = foo
end
end

result = interactor.call(foo: "baz", hello: "world")

expect(result.output).to eq("baz")
end

it "assigns absent positional arguments" do
interactor.class_eval do
def call(foo = "bar")
context.output = foo
end
end

result = interactor.call(hello: "world")

expect(result.output).to eq("bar")
end

it "raises an error for missing positional arguments" do
interactor.class_eval do
def call(foo)
context.output = foo
end
end

expect { interactor.call(hello: "world") }.to raise_error(ArgumentError)
end
end

context "keyword arguments" do
it "accepts required keyword arguments" do
interactor.class_eval do
def call(foo:)
context.output = foo
end
end

result = interactor.call(foo: "baz", hello: "world")

expect(result.output).to eq("baz")
end

it "accepts optional keyword arguments" do
interactor.class_eval do
def call(foo: "bar")
context.output = foo
end
end

result = interactor.call(foo: "baz", hello: "world")

expect(result.output).to eq("baz")
end

it "assigns absent keyword arguments" do
interactor.class_eval do
def call(foo: "bar")
context.output = foo
end
end

result = interactor.call(hello: "world")

expect(result.output).to eq("bar")
end

it "raises an error for missing keyword arguments" do
interactor.class_eval do
def call(foo:)
context.output = foo
end
end

expect { interactor.call(hello: "world") }.to raise_error(ArgumentError)
end
end

context "combination arguments" do
it "accepts required positional with required keyword arguments" do
interactor.class_eval do
def call(foo, hello:)
context.output = [foo, hello]
end
end

result = interactor.call(foo: "baz", hello: "world")

expect(result.output).to eq(["baz", "world"])
end

it "accepts required positional with optional keyword arguments" do
interactor.class_eval do
def call(foo, hello: "there")
context.output = [foo, hello]
end
end

result = interactor.call(foo: "baz", hello: "world")

expect(result.output).to eq(["baz", "world"])
end

it "accepts required positional and assigns absent keyword arguments" do
interactor.class_eval do
def call(foo, hello: "there")
context.output = [foo, hello]
end
end

result = interactor.call(foo: "baz")

expect(result.output).to eq(["baz", "there"])
end

it "accepts optional positional with required keyword arguments" do
interactor.class_eval do
def call(foo = "bar", hello:)
context.output = [foo, hello]
end
end

result = interactor.call(foo: "baz", hello: "world")

expect(result.output).to eq(["baz", "world"])
end

it "accepts optional positional with optional keyword arguments" do
interactor.class_eval do
def call(foo = "bar", hello: "there")
context.output = [foo, hello]
end
end

result = interactor.call(foo: "baz", hello: "world")

expect(result.output).to eq(["baz", "world"])
end

it "accepts optional positional and assigns absent keyword arguments" do
interactor.class_eval do
def call(foo = "bar", hello: "there")
context.output = [foo, hello]
end
end

result = interactor.call(foo: "baz")

expect(result.output).to eq(["baz", "there"])
end

it "assigns absent positional and accepts required keyword arguments" do
interactor.class_eval do
def call(foo = "bar", hello:)
context.output = [foo, hello]
end
end

result = interactor.call(hello: "world")

expect(result.output).to eq(["bar", "world"])
end

it "assigns absent positional and accepts optional keyword arguments" do
interactor.class_eval do
def call(foo = "bar", hello: "there")
context.output = [foo, hello]
end
end

result = interactor.call(hello: "world")

expect(result.output).to eq(["bar", "world"])
end

it "assigns absent positional and absent keyword arguments" do
interactor.class_eval do
def call(foo = "bar", hello: "there")
context.output = [foo, hello]
end
end

result = interactor.call

expect(result.output).to eq(["bar", "there"])
end
end
end
end