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

New matcher: `have_attributes` #571

Merged
merged 1 commit into from Jun 24, 2014

Conversation

@yelled3
Copy link

@yelled3 yelled3 commented Jun 10, 2014

see: #565

@myronmarston @JonRowe @cupakromer /cc

TODO's

  • update Changelog
@yelled3 yelled3 changed the title Added have_attributes matcher New matcher: `have_attributes` Jun 10, 2014
end

def respond_to_matcher
@respond_to_matcher ||= RespondTo.new(*expected.keys)

This comment has been minimized.

@yelled3

yelled3 Jun 10, 2014
Author

I'm sensing this will raise some objections :-)

This comment has been minimized.

@myronmarston

myronmarston Jun 10, 2014
Member

Nah, I think this is a nice way to leverage that logic :).

This comment has been minimized.

@JonRowe

JonRowe Jun 11, 2014
Member

Should we / does this check the arity of the methods?

@yelled3
Copy link
Author

@yelled3 yelled3 commented Jun 10, 2014

@myronmarston rubocop is not happy with me :-/

rubocop lib
Inspecting 43 files
..........................................C

Offenses:

lib/rspec/matchers.rb:579:9: C: Rename have_attributes to attributes?.
    def have_attributes(expected)
        ^^^^^^^^^^^^^^^

how can I silence this? do I need to remove/add a rule?

# @example
#
# expect(person).to have_attributes(:color => "red")
def have_attributes(expected)

This comment has been minimized.

@yujinakayama

yujinakayama Jun 10, 2014
Member

You can disable the offense report by RuboCop by adding an annotation comment as def have_attributes(expected) # rubocop:disable Style/PredicateName.

This comment has been minimized.

@yelled3
Copy link
Author

@yelled3 yelled3 commented Jun 10, 2014

it seems CI fails in:

  • 1.8.7
  • ree
  • jruby-18mode

seems to be related to:
https://github.com/rspec/rspec-expectations/blob/master/spec/rspec/matchers/built_in/include_spec.rb#L530-L538

anyone? :-)

it { is_expected.to have_attributes(:age => 32) }
it { is_expected.to have_attributes(:age => (a_value > 30) ) }
it { is_expected.to have_attributes(:name => "Myron", :age => 32) }
it { is_expected.to have_attributes(:name => a_string_starting_with("My"), :age => (a_value > 30) ) }

This comment has been minimized.

@myronmarston

myronmarston Jun 10, 2014
Member

You have an extra space here.


```ruby
Person = Struct.new(:name, :age)
person = Person.new("Myron", 32)

This comment has been minimized.

@myronmarston

myronmarston Jun 10, 2014
Member

Haha, I wasn't expecting you to use me as the example :). I think I'd slightly prefer a different example...

This comment has been minimized.

@yelled3

yelled3 Jun 10, 2014
Author

in the arts they it homage :-)
I'll replace it if you insist...

This comment has been minimized.

@JonRowe

JonRowe Jun 11, 2014
Member

I'd prefer something generic (especially as Myron has a slight preference for something else too)

it { is_expected.to have_attributes(:name => "Bob") }
it { is_expected.to have_attributes(:age => 10) }
# fails if any of the attributes doesn't match

This comment has been minimized.

@myronmarston

myronmarston Jun 10, 2014
Member

s/doesn't/don't/

#
# expect(person).to have_attributes(:color => "red")
#
# rubocop:disable Style/PredicateName

This comment has been minimized.

@myronmarston

myronmarston Jun 10, 2014
Member

I think you should put # rubocop:enable Style/PredicateName after the method so it is turned back on. @yujinakayama -- can you confirm?

This comment has been minimized.

@yujinakayama

yujinakayama Jun 10, 2014
Member

Yes. If the line contains only the annotation comment, the disablement of cop continues until next # rubocop:enable Style/PredicateName appears. On the other hand, if the comment is put after any token (as I suggested), it affects only the line. You can check the range of disablement by runnning rubocop --format progress --format disabled lib.


describe "expect(...).to have_attributes(key => matcher, key => matcher...)" do

it "passes when the matcher matches" do

This comment has been minimized.

@myronmarston

myronmarston Jun 10, 2014
Member

Should be it "passes when the matchers match".

expect(person).to have_attributes(:name => "Myron")
end

it "fails if target does not have_attributes any of the expected attributes" do

This comment has been minimized.

@myronmarston

myronmarston Jun 10, 2014
Member

s/have_attributes/have/


describe "expect(...).to_not have_attributes(with_one_attribute)" do

it "passes if target does not have_attributes any of the expected attributes" do

This comment has been minimized.

@myronmarston

myronmarston Jun 10, 2014
Member

s/have_attributes/have/

expect(person).to_not have_attributes(:age => 12)
end

it "fails if target have_attributes all of the expected attributes" do

This comment has been minimized.

@myronmarston

myronmarston Jun 10, 2014
Member

s/have_attributes/has/

end

context "for a string target" do
it "passes if target have_attributes expected" do

This comment has been minimized.

@myronmarston

myronmarston Jun 10, 2014
Member

I think this would be better phrased as: it "passes if target has the provided attributes".

This comment has been minimized.

@JonRowe

JonRowe Jun 11, 2014
Member

Isn't this duplicated from the shared examples anyway?

let(:matcher) { have_attributes(:name => "Myron", :age => 32) }
end

context "for a string target" do

This comment has been minimized.

@myronmarston

myronmarston Jun 10, 2014
Member

What does "for a string target" mean here? You are setting an expectation in a person, which is not a string...

end
end

describe "expect(...).to_not have_attributes(with_multiple_attributes)" do

This comment has been minimized.

@myronmarston

myronmarston Jun 10, 2014
Member

This doesn't really make sense as a nested example group under describe "expect(...).to have_attributes(with_multiple_attributes)".

end

context "for a string target" do
it "passes if target have_attributes expected" do

This comment has been minimized.

@myronmarston

myronmarston Jun 10, 2014
Member

I think this would be better phrased as: it "passes if target has the provided attributes".

expect(person).to have_attributes(:name => "Myron", :age => 32)
end

it "fails if target does not have_attributes any of the expected attributes" do

This comment has been minimized.

@myronmarston

myronmarston Jun 10, 2014
Member

s/have_attributes/have/


describe "expect(...).to_not have_attributes(with_multiple_attributes)" do

it "passes if target does not have_attributes any of the expected attributes" do

This comment has been minimized.

@myronmarston

myronmarston Jun 10, 2014
Member

s/have_attributes/have/

expect(person).to_not have_attributes(:name => "Myron", :age => 12)
end

it "fails if target have_attributes all of the expected attributes" do

This comment has been minimized.

@myronmarston

myronmarston Jun 10, 2014
Member

s/have_attributes/has/

# rubocop:disable Style/PredicateName
def have_attributes(expected)
BuiltIn::HaveAttributes.new(expected)
end

This comment has been minimized.

@myronmarston

myronmarston Jun 10, 2014
Member

We should add an aliased form of this matcher for use composed expressions:

expect { |probe|
  foo(&probe)
}.to yield_with_args(an_object_having_attributes(:name => "Jane", :age => 40))

I'm not sure what the best phrasing for the alias is, though. Most of our other aliases use the -ing form (which is why an_object_having_attributes is what I used above). Another possibility is an_object_with_attributes, which reads more naturally (to me, anyway) but is less consistent with the naming convention of the other aliases.

What do others think?

/cc @samphippen @xaviershay @JonRowe @soulcutter @cupakromer

This comment has been minimized.

@yelled3

yelled3 Jun 11, 2014
Author

@myronmarston what about:

  • an_object_with_attributes + single form * an_object_with_an_attribute
  • with_attributes + single form * with_an_attribute

This comment has been minimized.

@myronmarston

myronmarston Jun 11, 2014
Member

@yelled3 -- I'm not following...can you show example specs of what you are proposing?

This comment has been minimized.

@yelled3

yelled3 Jun 11, 2014
Author

i was simply suggesting possible aliases:

expect(person).to an_object_with_attributes(:name => "Myron", :age => 32)
expect(person).to an_object_with_attribute(:name => "Myron")

expect { |probe|
  foo(&probe)
}.to yield_with_args(an_object_having_attribute(:age => (a_value > 18)))


expect(person).to with_attributes(:name => "Myron", :age => 32)
expect(person).to with_an_attribute(:name => "Myron")

expect { |probe|
  foo(&probe)
}.to yield_with_args(with_attributes(:name => "Jane", :age => 40))

This comment has been minimized.

@myronmarston

myronmarston Jun 13, 2014
Member

I don't think with_attributes on its own makes sense. Either an_object_with_attributes or an_object_having_attributes make sense to me -- which we pick depends on if we favor consistency or more natural phrasing.

@JonRowe, what do you think?

This comment has been minimized.

@JonRowe

JonRowe Jun 14, 2014
Member

I don't think with_attributes on its own makes sense.

Agree, but just attributes(...) would but that's probably too generic, I have a minor preference for an_object_with_attributes(..) over an_object_having_attributes(..) but I'm ok with the latter for consistency.

This comment has been minimized.

@yelled3

yelled3 Jun 14, 2014
Author

which we pick depends on if we favor consistency or more natural phrasing.

if consistency means doing:

alias_matcher :an_object_ORIG_NAME, :ORIG_NAME

than by looking at other alias_matcher definitions, it seem like natural phrasing is mostly favored:

alias_matcher :an_object_existing, :exist
alias_matcher :an_object_matching, :match
alias_matcher :an_object_responding_to, :respond_to
alias_matcher :an_object_satisfying, :satisfy

so I vote for an_object_with_attributes

This comment has been minimized.

@myronmarston

myronmarston Jun 17, 2014
Member

if consistency means doing:

That's not what I mean by consistency. (In fact, I don't think we have any matcher aliases that use that form).

I mean that most of our matcher aliases use an -ing form:

➜  rspec-expectations git:(3-0-maintenance) ack "alias_matcher.*ing" lib/rspec/matchers.rb
    alias_matcher :a_block_changing,  :change
    alias_matcher :changing,          :change
    alias_matcher :a_collection_containing_exactly, :contain_exactly
    alias_matcher :containing_exactly,              :contain_exactly
    alias_matcher :a_range_covering, :cover
    alias_matcher :covering,         :cover
    alias_matcher :a_collection_ending_with, :end_with
    alias_matcher :a_string_ending_with,     :end_with
    alias_matcher :ending_with,              :end_with
    alias_matcher :an_object_existing, :exist
    alias_matcher :existing,           :exist
    alias_matcher :a_collection_including, :include
    alias_matcher :a_string_including,     :include
    alias_matcher :a_hash_including,       :include
    alias_matcher :including,              :include
    alias_matcher :an_object_matching, :match
    alias_matcher :a_string_matching,  :match
    alias_matcher :matching,           :match
    alias_matcher :a_block_outputting, :output
    alias_matcher :a_block_raising,  :raise_error do |desc|
    alias_matcher :raising,        :raise_error do |desc|
    alias_matcher :an_object_responding_to, :respond_to
    alias_matcher :responding_to,           :respond_to
    alias_matcher :an_object_satisfying, :satisfy
    alias_matcher :satisfying,           :satisfy
    alias_matcher :a_collection_starting_with, :start_with
    alias_matcher :a_string_starting_with,     :start_with
    alias_matcher :starting_with,              :start_with
    alias_matcher :a_block_throwing, :throw_symbol do |desc|
    alias_matcher :throwing,        :throw_symbol do |desc|
    alias_matcher :a_block_yielding_control,  :yield_control
    alias_matcher :yielding_control,          :yield_control
    alias_matcher :a_block_yielding_with_no_args,  :yield_with_no_args
    alias_matcher :yielding_with_no_args,          :yield_with_no_args
    alias_matcher :a_block_yielding_with_args,  :yield_with_args
    alias_matcher :yielding_with_args,          :yield_with_args
    alias_matcher :a_block_yielding_successive_args,  :yield_successive_args
    alias_matcher :yielding_successive_args,          :yield_successive_args

It's easier to guess what the matcher alias will be when we stick with a common convention like this.

than by looking at other alias_matcher definitions, it seem like natural phrasing is mostly favored:

Those are all examples of -ing phrasing. They do read pretty naturally, but there are other phrasings that are pretty natural, too:

  • an_object_that_matches("foo") rather than an_object_matching("foo")
  • an_object_that_responds_to(:foo) rather than an_object_responding_to(:foo)
  • etc.

I think I'm leaning towards an_object_having_attributes for consistency.

Thoughts from @samphippen @soulcutter @cupakromer @xaviershay @yujinakayama ?

This comment has been minimized.

@yelled3

yelled3 Jun 17, 2014
Author

OK.
are there any other issues?

This comment has been minimized.

@myronmarston

myronmarston Jun 19, 2014
Member

Nope, I think the alias is the last thing needed.

private

def perform_match
expected.all? do |attribute_key, attribute_value|

This comment has been minimized.

@myronmarston

myronmarston Jun 10, 2014
Member

This isn't going to work right in a case like this:

person = Person.new("Myron", 32)
expect(person).not_to have_attributes(:name => "Bob", :age => 32)

In this case, it will pass but should fail. As discussed on the original issue, in the negative form, it's an expectation that the object has none of the provided attributes, but your implementation is essentially, !all? { }, which will pass as long as at least one of the attributes does not match.

Please add a spec for this as well.

This comment has been minimized.

@JonRowe

JonRowe Jun 11, 2014
Member

Hmm, maybe the negative case is confusing then, because I'd have expected your example to pass...

This comment has been minimized.

@yelled3

yelled3 Jun 11, 2014
Author

funny - after all the discussion we had about this, I still ended up confused :-)

I liked @JonRowe's approach:

The inverse case is in my mind rather simple, as it's just boolean algebra.

note that we follow your suggestion then, {name: 'bob', age: 32} is a contradiction:

# A && B  == FALSE
expect(person).to have_attributes(:name => "Bob", :age => 32) # fail

# !(A && B) == !A || !B == TRUE
expect(person).not_to have_attributes(:name => "Bob", :age => 32) # fail

which is I think the source of confusion - since you would've expect that, for the most part, .not_to be the reverse of .to.

which is the default behaviour, when does_not_match? is not defined:

class NegativeExpectationHandler
def self.handle_matcher(actual, initial_matcher, message=nil, &block)
matcher = ExpectationHelper.setup(self, initial_matcher, message)
return ::RSpec::Matchers::BuiltIn::NegativeOperatorMatcher.new(actual) unless initial_matcher
!(does_not_match?(matcher, actual, &block) || ExpectationHelper.handle_failure(matcher, message, :failure_message_when_negated))
end
def self.does_not_match?(matcher, actual, &block)
if matcher.respond_to?(:does_not_match?)
matcher.does_not_match?(actual, &block)
else
!matcher.matches?(actual, &block)
end
end

This comment has been minimized.

@myronmarston

myronmarston Jun 11, 2014
Member

It's like respond_to:

expect(x).not_to respond_to(:foo, :bar)

respond_to is implemented so that this is treated as a shorter form of:

expect(x).not_to respond_to(:foo)
expect(x).not_to respond_to(:bar)

...meaning that if x responds to either foo or bar (or both), the example will fail. Likewise, this expectation:

expect(x).not_to have_attributes(:foo => 1, :bar => 2)

...should be treated as a shorter form of:

expect(x).not_to have_attributes(:foo => 1)
expect(x).not_to have_attributes(:bar => 2)

...meaning that the example will fail if either x.foo equals 1 or x.bar equals 2. But as implemented currently, the example will pass as long as one of these attributes does not equal the specified value.

This is how include, works, too. There are other matchers that are the same as well I (but I can't think of the off the top of my head).

This comment has been minimized.

@myronmarston

myronmarston Jun 11, 2014
Member

Another reason to treat it this way: it means that with each additional key/value pair, you are strengthening the expectation. It is specifying an additional thing about the object that must be true for the example to pass.

In contrast, as currently implemented, each additional key/value pair makes the expectation weaker:

# This will only pass if the name is _not_ Myron
expect(person).to have_attributes(:name => "Myron")

# ...but now it could pass if the name is Myron, as long as the age is not 32. (Or vice versa).
expect(person).to have_attributes(:name => "Myron", :age => 32)

Imagine an extreme case, where you pass 100 key/value pairs: the expectation means practically nothing. All that a passing example would mean is that at least 1 of the 100 attributes does not have the specified value. The other 99 could all have the values that you've just said you don't expect.

What use would that be?

This comment has been minimized.

@yelled3

yelled3 Jun 11, 2014
Author

good point.
thanks for the explanation.

@@ -0,0 +1,150 @@
require 'uri'

This comment has been minimized.

@myronmarston

myronmarston Jun 10, 2014
Member

Why are you requiring this?

# @return [Boolean]
def diffable?
true
end

This comment has been minimized.

@myronmarston

myronmarston Jun 10, 2014
Member

I noticed in the build that the diff is odd:

https://travis-ci.org/rspec/rspec-expectations/jobs/27226588#L275

If you have an idea for how to fix that, feel free -- otherwise, my advice is to make it not diffable for now.

This comment has been minimized.

@JonRowe

JonRowe Jun 11, 2014
Member

It's odd because Structs are enumerable and the differ is interpreting it as an array.

This comment has been minimized.

@yelled3

yelled3 Jun 11, 2014
Author

solved it, by doing:

module Composable

   def surface_descriptions_in(item)
     #...
     elsif Struct === item
         Hash[surface_descriptions_in(item.each_pair.to_a)]

WDYT? @myronmarston @JonRowe /cc

This comment has been minimized.

This comment has been minimized.

@myronmarston

myronmarston Jun 11, 2014
Member

I'm not a fan of special-casing Struct. Our examples here use a struct for simplicity but any kind of ruby object can be used. It'll be confusing when other types of objects are used. Thinking about this more broadly, the actual object is any arbitrary ruby object, while the expected is provided as a hash. I don't think it makes sense to do a textual diff of an arbitrary ruby object against a hash. Let's make the matcher not diffable for now. We can always revisit.

This comment has been minimized.

@yelled3

yelled3 Jun 11, 2014
Author

sounds good.

I thought, that was a specific issue with Struct...
surface_descriptions_in isn't tested enough (from what I've seen)
we should have a unit test for each kind of type.
see: https://github.com/rspec/rspec-expectations/blob/master/spec/rspec/matchers/composable_spec.rb

we can deal with surface_descriptions_in in another PR :-)

This comment has been minimized.

This comment has been minimized.

@yelled3

yelled3 Jun 12, 2014
Author

that was fast :-)
thanks @JonRowe

@myronmarston
Copy link
Member

@myronmarston myronmarston commented Jun 10, 2014

it seems CI fails in:

1.8.7
ree
jruby-18mode

It's failing because on 1.8, hashes do not preserve their ordering, and your expected failure message is making an assumption about the ordering. That assumption holds on 1.9 but not 1.8. You can see how we dealt with this for the include matcher:

}.to fail_matching(%r|expected #{hash_inspect :a => 1, :b => 2} not to include :a and :b|)

@myronmarston
Copy link
Member

@myronmarston myronmarston commented Jun 10, 2014

Well done, @yelled3! I'm excited to have this matcher be part of RSpec and your implementation is quite good.

@myronmarston
Copy link
Member

@myronmarston myronmarston commented Jun 10, 2014

One other issue I just thought of: the object could respond to the method but require arguments. As currently implemented, it'll raise an ArgumentError but I think a better approach is to add with(0).arguments on the end of the respond_to matcher you use.

@yelled3
Copy link
Author

@yelled3 yelled3 commented Jun 11, 2014

@myronmarston thanks for the feedback - more to come soon.

@@ -560,6 +560,28 @@ def exist(*args)
alias_matcher :an_object_existing, :exist
alias_matcher :existing, :exist

# Passes if actual's attributes pass all the expected attributes hash.
# This works for any with attribute methods.

This comment has been minimized.

@JonRowe

JonRowe Jun 11, 2014
Member

I found this confusing, how about:

Passes if actual's attribute values match the expected attributes hash. This works no matter how you define your attribute readers.

This comment has been minimized.

@yelled3

yelled3 Jun 11, 2014
Author

yeah, yours reads much better, thanks.

end

end

This comment has been minimized.

@JonRowe

JonRowe Jun 11, 2014
Member

Excess line ending, please cleanup.

This comment has been minimized.

@yelled3

yelled3 Jun 11, 2014
Author

@myronmarston shouldn't rubocop catch this?

it { is_expected.to have_attributes(:age => 32) }
it { is_expected.to have_attributes(:age => (a_value > 30) ) }
it { is_expected.to have_attributes(:name => "Jim", :age => 32) }
it { is_expected.to have_attributes(:name => a_string_starting_with("J"), :age => (a_value > 30) ) }

This comment has been minimized.

@myronmarston

myronmarston Jun 11, 2014
Member

This still has an extra space...

This comment has been minimized.

@myronmarston

myronmarston Jun 12, 2014
Member

Are you going to delete the extra space?

This comment has been minimized.

@JonRowe

JonRowe Jun 13, 2014
Member

Myron is almost as picky over extra spaces as I am over extra newlines ;)

This comment has been minimized.

@JonRowe

JonRowe Jun 13, 2014
Member

Actually I'm also picky about excess whitespace so...

This comment has been minimized.

This comment has been minimized.

@yelled3

yelled3 Jun 13, 2014
Author

not meaning to offend anyone - clearly this a troubling issue;
is there still an extra space?
I was copying the format from:
https://github.com/rspec/rspec-expectations/blob/master/features/built_in_matchers/include.feature#L31-L53

This comment has been minimized.

@JonRowe

JonRowe Jun 13, 2014
Member

The styles fine, you've just made a typo on the line above the first comment and inserted two spaces between the to and the have

it { is_expected.to have_attributes
it { is_expected.to  have_attributes

This comment has been minimized.

@myronmarston

myronmarston Jun 13, 2014
Member

Yeah it just looks odd since have_attributes is lined up everywhere except this one line.

This comment has been minimized.

@yelled3

yelled3 Jun 13, 2014
Author

image

fixed :-)


describe "expect(...).to_not have_attributes(with_multiple_attributes)" do

it "passes if target does not have none of the expected attributes" do

This comment has been minimized.

@myronmarston

myronmarston Jun 11, 2014
Member

"does not have none" is a double negative and doesn't really make sense here. I think you mean "passes if target has none of the expected attributes".

@yelled3
Copy link
Author

@yelled3 yelled3 commented Jun 12, 2014

I believe everything is fixed.
you're welcome for a final check.

@myronmarston @JonRowe @cupakromer /cc

@yelled3
Copy link
Author

@yelled3 yelled3 commented Jun 12, 2014

@myronmarston @JonRowe any idea why the ruby-head build is failing?

https://travis-ci.org/rspec/rspec-expectations/jobs/27402266

+bin/cucumber --strict
Using the default profile...
....................................undefined method `unpack' for nil:NilClass (NoMethodError)
/home/travis/build/rspec/bundle/ruby/2.2.0/gems/cucumber-1.3.15/lib/cucumber/ast/step.rb:81:in `text_length'
/home/travis/build/rspec/bundle/ruby/2.2.0/gems/cucumber-1.3.15/lib/cucumber/ast/step_collection.rb:62:in `block in max_line_length'
/home/travis/build/rspec/bundle/ruby/2.2.0/gems/cucumber-1.3.15/lib/cucumber/ast/step_collection.rb:62:in `map'
/home/travis/build/rspec/bundle/ruby/2.2.0/gems/cucumber-1.3.15/lib/cucumber/ast/step_collection.rb:62:in `max_line_length'
/home/travis/build/rspec/bundle/ruby/2.2.0/gems/cucumber-1.3.15/lib/cucumber/ast/has_steps.rb:53:in `max_line_length'
/home/travis/build/rspec/bundle/ruby/2.2.0/gems/cucumber-1.3.15/lib/cucumber/ast/has_steps.rb:49:in `source_indent'
/home/travis/build/rspec/bundle/ruby/2.2.0/gems/cucumber-1.3.15/lib/cucumber/ast/background.rb:40:in `accept'
/home/travis/build/rspec/bundle/ruby/2.2.0/gems/cucumber-1.3.15/lib/cucumber/ast/tree_walker.rb:64:in `block in visit_background'
/home/travis/build/rspec/bundle/ruby/2.2.0/gems/cucumber-1.3.15/lib/cucumber/ast/tree_walker.rb:170:in `broadcast'
@myronmarston
Copy link
Member

@myronmarston myronmarston commented Jun 12, 2014

I don't see a spec for the case where the object responds to the attribute method but requires arguments (I noticed you addressed that in the implementation, though). Can you add a spec for that?

@yelled3
Copy link
Author

@yelled3 yelled3 commented Jun 13, 2014

@myronmarston

Can you add a spec for that?

see latest changes :-)

Gemfile Outdated
# gem 'rubysl'
# end
#
# gem 'rubocop', "~> 0.23.0", :platform => [:ruby_19, :ruby_20, :ruby_21]

This comment has been minimized.

@myronmarston

myronmarston Jun 13, 2014
Member

Why the gem file changes?

This comment has been minimized.

@yelled3

yelled3 Jun 13, 2014
Author

sry...
git commit -am is a bad habbit

}.to fail_matching("expected #{object_inspect person} to respond to :color")
end

it "fails if target responds to the attribute but require arguments" do

This comment has been minimized.

@myronmarston

myronmarston Jun 13, 2014
Member

s/require/requires/

This comment has been minimized.

@yelled3
Copy link
Author

@yelled3 yelled3 commented Jun 13, 2014

@myronmarston i think we're good

}.to fail_matching("expected #{object_inspect person} to respond to :color")
end

it "fails if target responds to the attribute but requires arguments" do

This comment has been minimized.

@yelled3

yelled3 Jun 16, 2014
Author

@myronmarston

I don't see a spec for the case where the object responds to the attribute method but requires arguments (I noticed you addressed that in the implementation, though). Can you add a spec for that?

see bellow, for all uses cases

else
improve_hash_formatting(super)
end
end

This comment has been minimized.

@myronmarston

myronmarston Jun 18, 2014
Member

This and failure_message have duplicated logic. How about something like this?

def failure_message
  respond_to_failure_message_or { super }
end

def failure_message_when_negated
  respond_to_failure_message_or { super }
end

private

def respond_to_failure_message_or
  if respond_to_failed
    respond_to_matcher.failure_message
  else
    improve_hash_formatting(yield)
  end
end

This comment has been minimized.

@yelled3

yelled3 Jun 18, 2014
Author

done :-)

@yelled3
Copy link
Author

@yelled3 yelled3 commented Jun 19, 2014

@myronmarston @JonRowe
CI is all meseed up...

https://travis-ci.org/rspec/rspec-expectations/jobs/27939066
https://travis-ci.org/rspec/rspec-expectations/builds/27939065

+bin/rspec spec --backtrace --format progress --profile --format progress --out specs.out
/home/travis/build/rspec/rspec-expectations/spec/rspec/matchers/built_in/have_attributes_spec.rb:1: undefined method `describe' for main:Object (NoMethodError)
    from /home/travis/build/rspec/rspec-core/lib/rspec/core/configuration.rb:1055:in `load'
    from /home/travis/build/rspec/rspec-core/lib/rspec/core/configuration.rb:1055:in `load_spec_files'
    from /home/travis/build/rspec/rspec-core/lib/rspec/core/configuration.rb:1055:in `each'
    from /home/travis/build/rspec/rspec-core/lib/rspec/core/configuration.rb:1055:in `load_spec_files'
    from /home/travis/build/rspec/rspec-core/lib/rspec/core/runner.rb:97:in `setup'
    from /home/travis/build/rspec/rspec-core/lib/rspec/core/runner.rb:85:in `run'
    from /home/travis/build/rspec/rspec-core/lib/rspec/core/runner.rb:70:in `run'
    from /home/travis/build/rspec/rspec-core/lib/rspec/core/runner.rb:38:in `invoke'
    from /home/travis/build/rspec/rspec-core/exe/rspec:4
    from bin/rspec:12:in `load'
    from bin/rspec:12
The command "script/run_build" exited with 1.

@JonRowe
Copy link
Member

@JonRowe JonRowe commented Jun 19, 2014

No it's not, you need to use RSpec.describe at the top level because we disable monkey patching on our builds.

@yelled3
Copy link
Author

@yelled3 yelled3 commented Jun 19, 2014

No it's not, you need to use RSpec.describe at the top level because we disable monkey patching on our builds.

it was all RED - so I panicked :-)

thanks. @JonRowe

@JonRowe
Copy link
Member

@JonRowe JonRowe commented Jun 19, 2014

It would be :P

Adam Farhi
).to be_aliased_to(
have_attributes(:age => 32)
).with_description("an object having attributes {:age => 32}")
end

This comment has been minimized.

@yelled3

yelled3 Jun 23, 2014
Author

@myronmarston added the alias

myronmarston added a commit that referenced this pull request Jun 24, 2014
New matcher: `have_attributes`
@myronmarston myronmarston merged commit 6f975b0 into rspec:master Jun 24, 2014
1 check passed
1 check passed
continuous-integration/travis-ci The Travis CI build passed
Details
@myronmarston
Copy link
Member

@myronmarston myronmarston commented Jun 24, 2014

Thanks, @yelled3!

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Linked issues

Successfully merging this pull request may close these issues.

None yet

4 participants
You can’t perform that action at this time.