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

Modify new interactor's argument list to handle some cases #242

Merged
merged 3 commits into from
Nov 20, 2017

Conversation

tokichie
Copy link
Contributor

@tokichie tokichie commented Oct 28, 2017

Hi, I'm using hanami for my new project, and I love it. Thank you for a cool framework.
I've noticed that new interactor interface (from 1.1) cannot handle some argument patterns.
I propose a solution to handle such cases.

case 1: no keyword arguments
In this case, the actual arguments passed to #call is ['foo', 'bar', {}]
This can be avoided to change #call args to call(foo, bar, *) but i think it is not cool 🤔

class FooInteractor
  include Hanami::Interactor
  def call(foo, bar)
  end
end
[3] pry(main)> interactor = FooInteractor.new
=> #<FooInteractor:0x007f9d2797ef98>
[4] pry(main)> interactor.call('foo', 'bar')
ArgumentError: wrong number of arguments (given 3, expected 2)
from (pry):4:in `call'

case 2: no keyword arguments and an argument with #to_hash method
In this case, the last argument is converted into Hash automatically if it implements #to_hash.
Because Hanami::Entity implements #to_hash, this case may happen often, and makes the behavior unexpected 😢

class FooInteractor
  include Hanami::Interactor
  def call(foo, bar)
    p foo, bar
  end
end

class Foo < Hanami::Entity; end
class Bar < Hanami::Entity; end
[8] pry(main)> foo = Foo.new(foo: 'foo')
=> #<Foo:0x007fb0513f2380 @attributes={:foo=>"foo"}>
[9] pry(main)> bar = Bar.new(bar: 'bar')
=> #<Bar:0x007fb051369378 @attributes={:bar=>"bar"}>
[10] pry(main)> interactor.call(foo, bar)
#<Foo:0x007fb0513f2380 @attributes={:foo=>"foo"}>
{:bar=>"bar"}
^^^^^^^^^^^^^ Bar is converted into a hash
=> #<Hanami::Interactor::Result:0x00007fb0512db1b8 @success=true @payload={}>

  without kwargs: call(foo, bar)
  args with #to_hash method: call(some_entity)
  - removed **kwargs from arguments list
@cllns
Copy link
Member

cllns commented Oct 29, 2017

Thanks for this PR :)

@TiteiKo You worked on changing the args. What do you think about this change?

@jodosha jodosha requested a review from TiteiKo November 7, 2017 08:34
Copy link
Member

@jodosha jodosha left a comment

Choose a reason for hiding this comment

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

@tokichie Thanks for this PR. I manually tested these new cases and they work well.

The following script fails with hanami-utils 1.1.0 (aka master). But it works as expected with your branch.

require "bundler/setup"
require "hanami/interactor"

class FooInteractor
  include Hanami::Interactor

  def call(foo, bar)
    puts foo
    puts bar
  end
end

FooInteractor.new.call(1, 2)

class Entity
  def initialize(attributes)
    @attributes = attributes
  end

  def to_hash
    @attributes
  end
end

class BarInteractor
  include Hanami::Interactor

  def call(entity)
    puts entity.inspect
  end
end

entity = Entity.new(name: "Luca")
BarInteractor.new.call(entity)

class BazInteractor
  include Hanami::Interactor

  def call(foo:, bar: 2)
    puts foo
    puts bar
  end
end

BazInteractor.new.call(foo: 1)
BazInteractor.new.call(foo: 1, bar: 30)

begin
  BazInteractor.new.call(bar: 4)
rescue ArgumentError
  puts "This raises an error because `foo' is missing"
end

@jodosha jodosha added this to the v1.1.1 milestone Nov 7, 2017
@jodosha jodosha merged commit 6a3bc0d into hanami:master Nov 20, 2017
@tokichie tokichie deleted the fix-interactor-interface branch November 22, 2017 02:32
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants