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

Add methods #successful? and #failing? to Interactor #148

Merged
merged 1 commit into from Aug 12, 2016

Conversation

Projects
None yet
5 participants
@mgrachev
Contributor

mgrachev commented Jul 16, 2016

Example:

module Web::Controllers::Projects
  class Create
    include Web::Action

    params do
      param :project do
        param :name, presence: true
      end
    end

    def call(params)
      return unless params.valid?

      # Interactor
      create_project = CreateProject.new(params[:project]).call
      return if create_project.failure?

      redirect_to routes.projects_path
    end
  end
end
@coveralls

This comment has been minimized.

coveralls commented Jul 16, 2016

Coverage Status

Coverage increased (+0.002%) to 98.269% when pulling dd1109b on mgrachev:interactor-failure-method into 647f755 on hanami:master.

@joneslee85 joneslee85 added the feature label Jul 18, 2016

@joneslee85

This comment has been minimized.

Member

joneslee85 commented Jul 18, 2016

cc @hanami/core-team

I am 👍 with this convenient method

@jodosha

This comment has been minimized.

Member

jodosha commented Aug 10, 2016

@mgrachev What about:

return unless result.valid?

Which is similar to the line at the begin of the method:

return unless params.valid?
@cllns

This comment has been minimized.

Member

cllns commented Aug 10, 2016

I like this!

@jodosha Do you mean return unless result.success?

I prefer the implementation @mgrachev wrote. It will return true/false rather than true/nil. Any reason true/nil would be better?

@jodosha

This comment has been minimized.

Member

jodosha commented Aug 11, 2016

@cllns Yup, sorry there was a typo.

My suggestion is to make consistent how the guards are read in that method.

def call(params)
    return unless params.valid?

    # Interactor
    create_project = CreateProject.new(params[:project]).call
    return unless create_project.success? # This is my suggestion: it's consistent with the first line of this method.

    redirect_to routes.projects_path
end

What do you think?

@cllns

This comment has been minimized.

Member

cllns commented Aug 11, 2016

ah, I see. I thought you were saying return unless create_project.success? should be the body of the failure? mehod.

Yea, that looks good in this case but I don't really have a preference either way. I think .failure? is a useful convenience method though, for people who use Interactors in a different way (like outside of an Action, for a non-web usage).

Honestly, I like to avoid unless whenever possible, so I'd prefer to write the first line as return if params.invalid? (if params.invalid? were implemented) :D

@jodosha

This comment has been minimized.

Member

jodosha commented Aug 12, 2016

@mgrachev @joneslee85 @cllns I have to confess that I don't like the original choice of #success? because it reads bad on tests.

Minitest:

result.must_be :success?

RSpec:

expect(result).to be_success

I think we should improve that. My proposal is:

  1. Define #successful? method
  2. Make #success? an alias of #successful? for backward compatibility
  3. Introduce #failing? (instead of #failure?) as opposite of #successful?

What do you think?

@joneslee85

This comment has been minimized.

Member

joneslee85 commented Aug 12, 2016

@jodosha I like that proposal, to me adjective successful is much better in grammar.

@jodosha

This comment has been minimized.

Member

jodosha commented Aug 12, 2016

Let's go for it! 😉

@mgrachev mgrachev changed the title from Add method #failure? to Interactor to Add methods #successful? and #failing? to Interactor Aug 12, 2016

@coveralls

This comment has been minimized.

coveralls commented Aug 12, 2016

Coverage Status

Coverage increased (+0.002%) to 98.095% when pulling 29df251 on mgrachev:interactor-failure-method into 5038cb9 on hanami:master.

@mgrachev

This comment has been minimized.

Contributor

mgrachev commented Aug 12, 2016

@jodosha Done.

@jodosha jodosha added this to the next milestone Aug 12, 2016

@jodosha jodosha self-assigned this Aug 12, 2016

@jodosha

This comment has been minimized.

Member

jodosha commented Aug 12, 2016

@mgrachev Thank you very much. Merging it!

@jodosha jodosha merged commit 8df6846 into hanami:master Aug 12, 2016

2 checks passed

continuous-integration/travis-ci/pr The Travis CI build passed
Details
coverage/coveralls Coverage increased (+0.002%) to 98.095%
Details

@mgrachev mgrachev deleted the mgrachev:interactor-failure-method branch Aug 12, 2016

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment