dry-rb / dry-logic

Predicate logic with rule composition
https://dry-rb.org/gems/dry-logic/
MIT License
179 stars 66 forks source link

Switch order of arguments in respond_to predicate #56

Closed waiting-for-dev closed 5 years ago

waiting-for-dev commented 5 years ago

This is the order that dry-types expect, and it makes more sense having function currying in mind

@solnic this should be merged to master. It was my mistake yesterday.

References #55

solnic commented 5 years ago

Ugh of course. I missed that too 😄 The build failed though. Maybe you gotta undef :respond_to? there.

waiting-for-dev commented 5 years ago

Hmm it doesn't have nothing to do with respond_to? predicate. Am I wrong?

  1) Dry::Logic::Rule arity specialization arbitrary arity generates correct arity
     Failure/Error: expect(rule.method(:call).arity).to be(arity - curried)

       expected #<Integer:1> => 0
            got #<Integer:-1> => -1

       Compared using equal?, which compares object identity,
       but expected and actual are not the same object. Use
       `expect(actual).to eq(expected)` if you don't care about
       object identity in this example.
     # ./spec/unit/rule_spec.rb:190:in `block (4 levels) in <top (required)>'
Finished in 0.11962 seconds (files took 0.27258 seconds to load)
206 examples, 1 failure
Failed examples:
rspec ./spec/unit/rule_spec.rb:189 # Dry::Logic::Rule arity specialization arbitrary arity generates correct arity
waiting-for-dev commented 5 years ago

Yep, this fails when rand(20) returns 0, also removing respond_to predicate:

    describe 'arbitrary arity' do
      let(:arity) { rand(20) }
      let(:curried) { arity.zero? ? 0 : rand(arity) }

      let(:options) { { args: [1] * curried, arity: arity } }
      let(:predicate) { double(:predicate) }

      it 'generates correct arity' do
        expect(rule.method(:call).arity).to be(arity - curried)
        expect(rule.method(:[]).arity).to be(arity - curried)
      end
    end

Anyway, if you want I can tackle it in another PR.

waiting-for-dev commented 5 years ago

Polluting this PR but.. does it make sense an arity of 0 there? Couldn't we just rand(1..20)?

solnic commented 5 years ago

does it make sense an arity of 0 there? Couldn't we just rand(1..20)?

it does not, this spec needs to be fixed

Anyway, if you want I can tackle it in another PR.

Please do

waiting-for-dev commented 5 years ago

@solnic done in #57

waiting-for-dev commented 5 years ago

Ok, I rebased it. Build clean :smile: