-
-
Notifications
You must be signed in to change notification settings - Fork 164
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Refactor deduper and write additional tests #649
Conversation
TBD: I'd like to add a function in the deduper middleware to check which of the two items is prefered when a dupe is found. This would allow us to pick WOF records over Geonames, and OA over OSM, etc. |
* @returns {boolean} | ||
* @throws {Error} | ||
*/ | ||
function isDiffLayer(item1, item2) { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
the method signature of this function is not very intuitive, it's called isDiffLayer
which I think is a great name if it returns a boolean value.
looking at the calling code it seems as though the return value is not actually used and the functions main purpose is validation via throw
.
a bit nit-picky, but would this function name be better as validateDiffLayer
(which returns nothing and throws on error) or isDiffLayer
(which does not throw and returns bool so the caller can handle the error case)?
the same question applies to all the is*
functions here.
Thanks @missinglink for the helpful feedback. Right on and not at all nitpicky. 👍 I think I've addressed all your comments. |
Very helpful description and I think this will solve our deduping issues. |
Thanks all for feedback. Merging this and will create a separate issue for the |
Refactored the deduper. Here's what got done:
layer
,parent.*_id
,address_parts.*
, andname.*
zip
if both records have it set, otherwise ignoreparent.*_id
properties, skip the check where *===item.layer
name.*
only expectname.default
to match, but only check additional languages if both items have it set, otherwise ignoreFixes #604