Skip to content

Override Sparsam::Struct#hash method#25

Open
marionzualo wants to merge 3 commits intoairbnb:mainfrom
marionzualo:mn-struct-hash
Open

Override Sparsam::Struct#hash method#25
marionzualo wants to merge 3 commits intoairbnb:mainfrom
marionzualo:mn-struct-hash

Conversation

@marionzualo
Copy link
Copy Markdown

@marionzualo marionzualo commented Mar 11, 2020

This ensures that different Set objects with Thrift objects having the same value are considered equal.

I added a spec but I wasn't able to run it in my local environment.

I get the following error

`require': cannot load such file -- sparsam_native (LoadError)

@pariser

This ensures that different Set objects with Thrift objects having the same
value are considered equal.
Copy link
Copy Markdown
Contributor

@pariser pariser left a comment

Choose a reason for hiding this comment

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

It looks like struct_fields should be deterministic based on this class_eval method signature, so that's good!

While we're at it, WDYT about adding other equality tests like us1 == us1, [us1] == [us2], { us1 => true } == { us2 => true }?

Comment thread spec/sparsam_spec.rb Outdated
it "respects equality of Sets" do
us1 = US.new
us2 = US.new
expect(Set[us1]).to eq(Set[us2])
Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

Looks like sparsam is not updated to use expect rspec syntax

Suggested change
expect(Set[us1]).to eq(Set[us2])
Set[us1].should == Set[us2]

would hopefully fix the assertion

Comment thread spec/sparsam_spec.rb Outdated
subdata.id_s.should == "id_s default"
end

it "respects equality of Sets" do
Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

Don't know too much about the way sparsam's tests are structured, but it seems strange to nest this within describe Sparsam::Serializer block

Copy link
Copy Markdown
Author

Choose a reason for hiding this comment

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

good call. I missed this.

@marionzualo
Copy link
Copy Markdown
Author

@pariser Thanks for the feedback! I implemented it and the tests are passing now. PTAL

@andyfangdz
Copy link
Copy Markdown
Contributor

To run tests, run bundle exec rake build_ext

@crazysona
Copy link
Copy Markdown

Hi @marionzualo , are you still working on this PR? I also encountered the same problem, two sets are not equal due to the hash are not overridden as well as eql?

Comment thread lib/sparsam/struct.rb

def hash
struct_fields.map do |fid, data|
data.hash
Copy link
Copy Markdown

@crazysona crazysona Mar 22, 2022

Choose a reason for hiding this comment

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

I think you need to use send(name_to_accessors(data[:name]).reader).hash

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

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

4 participants