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

Update ordered_containers.md #870

Open
wants to merge 1 commit into
base: master
Choose a base branch
from
Open

Conversation

SyxP
Copy link

@SyxP SyxP commented Sep 1, 2023

  1. Fixed incorrect example code in OrderedSet (Note that this code doesn't run as the various irrational numbers are not defined. Previously it was just a method error.)

  2. Added clarification on equality. The behaviour was raised as a point of confusion in a Slack thread. I just documented the current existing behaviour.

1) Fixed incorrect example code in `OrderedSet` (Note that this code doesn't run as the various irrational numbers are not defined.)

2) Added clarification on equality. The behaviour was raised as a point of confusion in a Slack thread. I just documented the current existing behaviour.
@jariji
Copy link

jariji commented Sep 1, 2023

This is documenting rather than fixing bad behavior IMO.

For any type T, x::T and y::T should not have x == y unless x and y agree on all aspects of their public API. Since order is part of the public API of Ordered containers, they should not be ==(::T, ::T) unless they agree on order.

@SyxP
Copy link
Author

SyxP commented Sep 1, 2023

I agree, I don't like how this works. But as we had a discussion on Slack, this is actually how someone might prefer the behaviour to be.

I make no judgement on whether this is the "correct" behaviour. But as of now this is what is inside the library, and I document it to prevent confusion. I will close the PR if there is another successfully merged PR augmenting the behaviour.

@StephenVavasis
Copy link
Contributor

I don't think ordered_set is part of this repository. I believe that it is in the JuliaCollections/OrderedCollections.jl repository.

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