Sorting JSON arrays by toString removed#51
Sorting JSON arrays by toString removed#51x87-va wants to merge 2 commits intotwitter-archive:masterfrom
Conversation
Current coverage is 35.02% (diff: 100%)@@ master #51 diff @@
==========================================
Files 29 29
Lines 789 788 -1
Methods 769 753 -16
Messages 0 0
Branches 20 35 +15
==========================================
- Hits 277 276 -1
Misses 512 512
Partials 0 0
|
| case JsonToken.START_ARRAY => | ||
| node.elements.toSeq.map { | ||
| element => lift(element) | ||
| } sortBy { _.toString } |
There was a problem hiding this comment.
Sorting is meant to prevent false positives caused by inconsistent orders in which the same Set can be rendered as a Json array. Since Json does not have any notion of Set we have to sort just in case the collection may have originally been a Set. The down side is that this creates a blind spot for OrderingDifferences in real lists. i.e. we have false negatives instead of false positives.
There was a problem hiding this comment.
Sorting in our case creates a lot of PrimitiveDifferences (30% - 50%) instead of OrderingDifferences.
There was a problem hiding this comment.
That's weird. Can you share a bit more about what the differences look like. By definition, if all you have are ordering differences then there should be no differences left after sorting. This might be a bug that requires a different fix.
There was a problem hiding this comment.
I made a demo for this case. Try to send 10 requests https://fifth-howl-149214.appspot.com/_ah/api/diff/v1/reply through Diffy.
|
Виталий Левашов seems not to be a GitHub user. You need a GitHub account to be able to sign the CLA. If you have already a GitHub account, please add the email address used for this commit to your account. You have signed the CLA already but the status is still pending? Let us recheck it. |
4 participants
When lifting nested JSON arrays diffy looses original ordering and generates diffs.