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

Fix sorted map always using insertion order when created as mutable #70

Open
wants to merge 4 commits into
base: master
Choose a base branch
from

Conversation

AreeseDT
Copy link

Pass compare function to sorted map when calling empty using mutable overload.

@axefrog
Copy link
Member

axefrog commented May 18, 2019

Hey thanks for the PR. Not ignoring you, you just caught me in a busy period and I need to reclone the repo and get up to date. I notice there is a build failure in Travis CI... if you are so inclined, perhaps you could take a look at the build log and let me know if you can see if it something that might be a problem?

@AreeseDT
Copy link
Author

@axefrog The build errors have been resolved

@eliranek1
Copy link
Contributor

@AreeseDT, what's the use case that this is solving? I see that this is pretty old now, is this still necessary?
also, can you please add some UTs?

@AreeseDT
Copy link
Author

@eliranek1 When creating a mutable empty sorted map, the value of the comparator is never set. This means every mutable empty sorted map is always sorted in insertion order. The expected behavior of a mutable empty sorted map is that any items added are sorted by the provided comparator.

It would appear that this is a bug seeing as there are two function signature overloads with mutable: boolean|MutationContext and a ComparatorFn<SortedMapEntry<K, V, undefined>> parameter. The presence of the ComparatorFn<SortedMapEntry<K, V, undefined>> parameter shouldn't be ignored completely for mutable sorted maps correct?

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.

None yet

3 participants