solnic / virtus

[DISCONTINUED ] Attributes on Steroids for Plain Old Ruby Objects
MIT License
3.77k stars 229 forks source link

Specs are failing #322

Closed jpmoral closed 9 years ago

jpmoral commented 9 years ago

Several specs are failing with undefined method 'first' for #<Virtus::Attribute>

This seems to be because of this change in the equalizer gem: old:

def ==(other)
  other = coerce(other) if respond_to?(:coerce, true)
  other.is_a?(self.class) && cmp?(__method__, other)
end

new:

def ==(other)
  other = coerce(other).first if respond_to?(:coerce, true) # added call to 'first'
  other.is_a?(self.class) && cmp?(__method__, other)
end

The spec trips up on trying to compare Virtus::Attributes, which don't have a first method defined. I'd be happy to attempt a fix if I knew more about each project.

solnic commented 9 years ago

We need our own Attribute#== method. Equalizer calls #coerce internally which in case of virtus attributes doesn't do the same thing as, let's say, Float#coerce. This should fix the specs and the behavior of equality method too.

jpmoral commented 9 years ago

Is it as simple as going back to not using the equalizer gem?

On a side note, changing how the specs are written from:

expect { subject }.to change { object.reset.to_a }.from(attributes).to([ attribute ])

to:

expect(object.reset.to_a).to eq attributes
subject
expect(object.reset.to_a).to eq [attribute]

gets them passing. It's much more difficult to understand though.

solnic commented 9 years ago

We're discussing how to solve this on the Equalizer side, see related issue.

novikserg commented 9 years ago

So I tried to look into it and it seems like we can implement a simple own matcher for comparing Virtus::Attribute models (for Attribute model type and option fields matching), but I thought I would wait for some movement on Equalizer side. Are you working on it already? If not, I would love to discuss it and help. Actually, I implemented Attribute#== method already - probably I can open a PR and we can merge if you like it, at least specs will be passing now (and some related bugs will be fixed I guess, since there should be some unreported ones related to that, if specs are failing). After that we can either wait for Equalizer fix or start using Virtus's own matchers everywhere (this will also remove the dependency, but that's a pretty good amount of potentially unnecessary work). Tell me your thoughts please, and nice project there!

solnic commented 9 years ago

@novikserg you may want to check out this thread: https://github.com/dkubb/equalizer/pull/18 I'm not sure how to proceed with this, currently leaning towards going back to the built-in equalizer that was deprecated in 1.0.0 but now for pragmatic reasons I feel like it could be brought back (esp. that it would remove the need for values .. end block in value_object extension.

novikserg commented 9 years ago

@solnic Alright, thanks for the info, I will send a PR in the following week or two.

jpmoral commented 9 years ago

Specs are fixed, closing.