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

add eql? and hash methods to table object for using objects as hash keys #31

Merged
merged 4 commits into from
Nov 5, 2018

Conversation

nhemsley
Copy link
Contributor

this makes airrecord objects usable as hash keys, before this change, each new add creates distinct key entries (unless it is the same actual ruby object).

This may not be wanted behaviour, but it should still detect records with dirty fields (serializable_fields.hash) and return not equal

Cheers for the library!

@nhemsley
Copy link
Contributor Author

nhemsley commented Aug 30, 2018

Actually I will add a couple of tests for this if you would like to merge:

test hash insertion doesnt cause duplicate keys & entries
test that changing a field causes eql? to fail
test that changing a field causes hashes to be nonequal

@sirupsen
Copy link
Owner

sirupsen commented Nov 4, 2018

Can you fix the merge conflict? Then we can get this in :)

@nhemsley
Copy link
Contributor Author

nhemsley commented Nov 4, 2018

I will look at this soon.

@Meekohi
Copy link
Collaborator

Meekohi commented Nov 4, 2018

Merge conflict and some of the tests are still using

walrus = Walrus.new("Name": "Wally") # Uses symbol -- no longer supported
walrus = Walrus.new("Name" => "Wally") # Use exact string

@nhemsley
Copy link
Contributor Author

nhemsley commented Nov 5, 2018

OK, merge conflict fixed. Please merge. Thanks.

@sirupsen sirupsen merged commit 6d557e3 into sirupsen:master Nov 5, 2018
@sirupsen
Copy link
Owner

sirupsen commented Nov 5, 2018

This'll get released with 1.0.0, thanks!

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.

3 participants