scala-collection-contrib
scala-collection-contrib copied to clipboard
scala
Issue 239 Fix
Addresses this issue: Possible bug in implementation of MapDecorator.mergeByKeyWith.
Test added to reproduce the bug in: ~~bcc2b6efedcb97ae329b792940b52e44ef1db06e~~4fe84ca7370f05059817fd333f72719d33fb4652.
Fix to make test pass in: ~~5088ed6ac8a23e3a9987491bf05f334d05cdfc74~~8c8d67bd323f7a9bf2b786b5b55c3349fa52f7f8.
The second commit is a bit messy - let me tidy up the history a little for the sake of clarity and force-push again...
@SethTisue , @Philippus This PR is awaiting workflow approval from a maintainer. Could somebody take a look if they have a moment, please?
I see the PR is unsquashed, but I don't know what local policy is for this repo.
The test is interesting in that it demonstrates the old behavior. Is such a test meaningful after the fix? That is, it becomes a test where one asks sometime in the future, Why did they ever think this test could fail? It tests for apples instead of oranges.
Nice catch. Does this mean no one uses this code? Worth noting that the original is surprisingly hard to read, in part because
Option#foldis obfuscating.
Well, I certainly use the code, although in the meantime I’ve copied the fixed code locally.😊
EDIT: where are my manners? I should say from the start, thanks for the review, @som-snytt and @SethTisue.
I see the PR is unsquashed, but I don't know what local policy is for this repo.
I was aiming to show the test failing first, then passing with the fix - I like to split my work into testing, implementing, refactoring, tidying up to avoid one big PR - although this is just a minnow anyway. It’s no problem from my side if you want to squash the merge if you’re happy with the PR.
The test is interesting in that it demonstrates the old behavior. Is such a test meaningful after the fix? That is, it becomes a test where one asks sometime in the future, Why did they ever think this test could fail? It tests for apples instead of oranges.
I was thinking of the test as being a bug reproduction test, but wrote it as two scenarios to show the simpler situation first where values don’t collide, followed by the one where they do.
Depending on how you philosophise about testing, you might say that the second case is a case of “let’s show that the SUT does not make my breakfast / make nuisance phone calls / launch nuclear missiles”, or instead you could say that preservation of the values is part of its wider contract.
I take the latter view, but if it’s a dealbreaker, how about we split the test? FWIW, I saw this repository as more of a fast-and-loose affair, so I just got something in to support the fix.
@som-snytt / @SethTisue Another bump for a workflow approval, please. Next time I won't fork the repository to submit a PR, lesson learned...