collection_test: add tests for SortedMap.slice #55
Conversation
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
I'm seeing weird behavior from SortedMap.slice and .sliceIndices. I think the docs, tests, and/or code is wrong. For lists, .slice is documented to take [lower, upper) bounds. But for sorted maps, .slice apparently takes [lower, upper]: both inclusive. That's kind of weird in itself--possibly it's shaped this way because it's not always clear how to construct a successor to the max element. The code certainly looks like it's intended to be both inclusive. Except it doesn't actually *do* that. Slicing to 0, 0 returns the *entire* map somehow. Slicing to 1, 1 yields the empty map. Slicing to 3, 8 yields just 3, and skips 4. There were no tests for .slice--only .sliceIndices, so I added some. Commit 65e43ad says that it fixed the tests for sliceIndices to reflect their inclusive, inclusive behavior. but the commit actually does the opposite: it makes the test for inclusive, exclusive. We should align the tests and code to whatever the behavior is supposed to be. We should also document the inclusive/exclusive behavior for sliceIndices etc. It's not actually written down, and that's terribly confusing.
|
Hey @ztellman, is this worth merging? |
This method was broken when iterators had different numbers of elements.
|
Yes, sorry for leaving this unattended for so long. I still need to carve off all the durable-related code and cut an |
|
No worries! I'm still expanding bifurcan-clj too--getting all kinds of use lately. Thanks again for writing this. :-) |
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
I'm seeing weird behavior from SortedMap.slice and .sliceIndices. I think the docs, tests, and/or code is wrong.
For lists, .slice is documented to take [lower, upper) bounds. But for sorted maps, .slice apparently takes [lower, upper]: both inclusive. That's kind of weird in itself--possibly it's shaped this way because it's not always clear how to construct a successor to the max element. The code certainly looks like it's intended to be both inclusive. Except it doesn't actually do that. Slicing to 0, 0 returns the entire map somehow. Slicing to 1, 1 yields the empty map. Slicing to 3, 8 yields just 3, and skips 4.
There were no tests for .slice--only .sliceIndices, so I added some. Commit 65e43ad says that it fixed the tests for sliceIndices to reflect their inclusive, inclusive behavior. but the commit actually does the opposite: it makes the test for inclusive, exclusive.
We should align the tests and code to whatever the behavior is supposed to be. We should also document the inclusive/exclusive behavior for sliceIndices etc. It's not actually written down, and that's terribly confusing.