Fix colliding fuzzy matches #8

Closed
wants to merge 2 commits into
from

Conversation

Projects
None yet
3 participants
@unconed
Contributor

unconed commented Jan 30, 2014

Diffing the following two arrays shows "c" incorrectly being added to the first object.

a.json:

[
{ "name": "Foo", "a": 3, "b": 1 }
]

b.json

[
{ "name": "Foo", "a": 3, "b": 1 },
{ "name": "Foo", "a": 3, "b": 1, "c": 1 }
]

In scalarize, it maps a target array's objects to matching source objects, but it doesn't check whether the mapping exists already.

    else if fuzzyOriginals && (bestMatch = findMatchingObject(item, index, fuzzyOriginals)) && bestMatch.score > 40
      originals[bestMatch.key] = item;

The proposed change passes all existing mappings into findMatchingObject and does the check there so that an alternate match can be found.

@unconed

This comment has been minimized.

Show comment Hide comment
@unconed

unconed Jan 31, 2014

Contributor

After some more real world testing, it seems better to just drop fuzzy match collisions altogether. Otherwise, you risk a low-scoring match hogging an item for which a much higher scoring match exists, and this can cascade down the array to bloat the diff.

Contributor

unconed commented Jan 31, 2014

After some more real world testing, it seems better to just drop fuzzy match collisions altogether. Otherwise, you risk a low-scoring match hogging an item for which a much higher scoring match exists, and this can cascade down the array to bloat the diff.

@andreyvit

This comment has been minimized.

Show comment Hide comment
@andreyvit

andreyvit Jan 31, 2014

Owner

I see. Can you please add a runnable test for that as well? And yes, as long as the existing tests pass and new tests are added, I'll be happy to merge any improvements. :-)

Owner

andreyvit commented Jan 31, 2014

I see. Can you please add a runnable test for that as well? And yes, as long as the existing tests pass and new tests are added, I'll be happy to merge any improvements. :-)

@unconed

This comment has been minimized.

Show comment Hide comment
@unconed

unconed Feb 21, 2014

Contributor

I can't figure out how to run your tests. Either way, I've provided the bug report, the isolated example, and the patch. Do you really need it with a cherry on top too?

Contributor

unconed commented Feb 21, 2014

I can't figure out how to run your tests. Either way, I've provided the bug report, the isolated example, and the patch. Do you really need it with a cherry on top too?

@unconed unconed closed this Feb 21, 2014

@andreyvit

This comment has been minimized.

Show comment Hide comment
@andreyvit

andreyvit Feb 22, 2014

Owner

Yes, please. :P

Seriously though, did npm test not work for you? I probably won't have time to update this package soon, so I rely on contributors like you to move it forward.

Owner

andreyvit commented Feb 22, 2014

Yes, please. :P

Seriously though, did npm test not work for you? I probably won't have time to update this package soon, so I rely on contributors like you to move it forward.

@andreyvit andreyvit reopened this Feb 22, 2014

@tsibley

This comment has been minimized.

Show comment Hide comment
@tsibley

tsibley Apr 11, 2014

Contributor

Running npm test in a git clone produces errors about not finding ./node_modules/mocha/bin/mocha. Once I install mocha locally with npm install mocha and re-run npm test, I get:

> json-diff@0.3.1 test /Users/trsibley/src/json-diff
> ./node_modules/mocha/bin/mocha

  0 passing (3ms)

which implies it's not running or even finding either of the test scripts. If I try to run mocha directly with ./node_modules/mocha/bin/mocha test/*.coffee, I get errors because mocha 1.7.x doesn't support coffee-script by default or something.

Anyway, after much trial and error, I get npm test working with the changes in PR #10.

Contributor

tsibley commented Apr 11, 2014

Running npm test in a git clone produces errors about not finding ./node_modules/mocha/bin/mocha. Once I install mocha locally with npm install mocha and re-run npm test, I get:

> json-diff@0.3.1 test /Users/trsibley/src/json-diff
> ./node_modules/mocha/bin/mocha

  0 passing (3ms)

which implies it's not running or even finding either of the test scripts. If I try to run mocha directly with ./node_modules/mocha/bin/mocha test/*.coffee, I get errors because mocha 1.7.x doesn't support coffee-script by default or something.

Anyway, after much trial and error, I get npm test working with the changes in PR #10.

@andreyvit

This comment has been minimized.

Show comment Hide comment
@andreyvit

andreyvit Dec 16, 2014

Owner

Hey, so do you guys want to provide a test for this so that the pull request can be merged?

Owner

andreyvit commented Dec 16, 2014

Hey, so do you guys want to provide a test for this so that the pull request can be merged?

@andreyvit

This comment has been minimized.

Show comment Hide comment
@andreyvit

andreyvit Aug 20, 2017

Owner

Merged PR #13 which incorporates this one, so closing this PR.

Owner

andreyvit commented Aug 20, 2017

Merged PR #13 which incorporates this one, so closing this PR.

@andreyvit andreyvit closed this Aug 20, 2017

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment