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

Allow comparison against iterables #9

Closed
wants to merge 1 commit into from

Conversation

jimrthy
Copy link
Contributor

@jimrthy jimrthy commented Sep 29, 2015

For issue #8: this affected both strings and arbitrary lists

For issue zhemao#8: this affected both strings and arbitrary lists
@zhemao
Copy link
Owner

zhemao commented Sep 29, 2015

This seems to conflict a bit with your other pull request. Now empty vectors and empty dictionaries are no longer equivalent. Should ImmutableVector also be made to inherit from collections.Mapping?

@jimrthy
Copy link
Contributor Author

jimrthy commented Sep 30, 2015

Hmm...I wouldn't expect an empty ImmutableVector to be equal to an empty ImmutableDict, since [] != {}. I can make that work if you like (especially since it's obviously this way for a reason--I totally missed this scenario), but it seems like that would imply that ImmutableDict({0: 'a', 1: 'b', 2: 'c'}) == ImmutableVector(['a', 'b', 'c']). That almost implies they should both be == 'abc', which definitely seems wrong.

I also definitely don't think an ImmutableVector should implement containers.Mapping. That would imply that you can use things like strings for keys. That's the problem I was really trying to avoid with this: an exception from comparing a string/list against an ImmutableDict w/ the same length.

Thanks!

@zhemao
Copy link
Owner

zhemao commented Sep 30, 2015

I'll just change the unittest and merge this in then.

@zhemao zhemao closed this Sep 30, 2015
@jimrthy jimrthy deleted the bug/string-compare branch October 5, 2015 17:11
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.

2 participants