Skip to content

map: Make Eq comparisons ordering-aware. #154

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

Closed
wants to merge 1 commit into from

Conversation

emilio
Copy link

@emilio emilio commented Sep 8, 2020

This is a breaking change. Not sure if you really want to take this but
I think it's the right thing to do.

This is a breaking change. Not sure if you really want to take this but
I think it's the right thing to do.

Fixes indexmap-rs#153
@cuviper
Copy link
Member

cuviper commented Sep 8, 2020

This is a breaking change.

I've added it to #135 to consider for 2.0.

In the meantime, we could add a method for ordered_eq, but it's just:

map1.len() == map2.len() && map1.iter().eq(&map2)

@emilio
Copy link
Author

emilio commented Sep 8, 2020

Yup, agree, ordered_eq is probably not quite worth it.

That seems reasonable, thank you!

@emilio
Copy link
Author

emilio commented Sep 8, 2020

Do you want to leave this PR open? Or should I close it?

@cuviper
Copy link
Member

cuviper commented Sep 8, 2020

Let's close -- it will be here when we're ready for 2.0, but that's not really happening yet.

@cuviper cuviper closed this Sep 8, 2020
assert_eq!(&set_a ^ &set_b, set_c);
assert_eq!(&set_b ^ &set_a, set_c);
assert_ne!(&set_b ^ &set_a, set_c);
Copy link
Member

Choose a reason for hiding this comment

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

when this is revived later, this change does not preserve the intent of the test. It can of course be fleshed out when/if relevant later.

@bluss
Copy link
Member

bluss commented Sep 9, 2020

Thanks for filing. Not a bug since it's the intentional design, but worth discussing for sure. I think a method for ordered eq would be worth it.

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

Successfully merging this pull request may close these issues.

3 participants