Issue 239 Fix #244
Issue 239 Fix #244
Conversation
sageserpent-open
force-pushed
the
issue-239-MapDecorator-mergeByKeyWith
branch
from
5088ed6
to
650f115
Compare
|
The second commit is a bit messy - let me tidy up the history a little for the sake of clarity and force-push again... |
sageserpent-open
force-pushed
the
issue-239-MapDecorator-mergeByKeyWith
branch
from
650f115
to
8c8d67b
Compare
|
Refreshed the PR and edited the original description accordingly. |
…used to control scope.
|
@SethTisue , @Philippus This PR is awaiting workflow approval from a maintainer. Could somebody take a look if they have a moment, please? |
|
|
||
| Assert.assertEquals("Expect the same keys to appear in the join taken either way around.", ourChanges.mergeByKey(theirChanges).keySet, theirChanges | ||
| .mergeByKey(ourChanges) | ||
| .keys) |
There was a problem hiding this comment.
This compares keySet and keys. Does it matter? Without a resolution to scala/scala#10766 it's fruitless to speculate.
There was a problem hiding this comment.
Oops - yes, let me fix this…
|
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. |
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 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.
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. |
…t using `.keySet` to express a map's keys.
|
@som-snytt / @SethTisue Another bump for a workflow approval, please. Next time I won't fork the repository to submit a PR, lesson learned... |
Addresses this issue: Possible bug in implementation of MapDecorator.mergeByKeyWith.
Test added to reproduce the bug in:
bcc2b6e4fe84ca.Fix to make test pass in:
5088ed68c8d67b.