Skip to content
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

Add FingerprintDiffer #219

Open
wants to merge 1 commit into
base: master
Choose a base branch
from
Open

Add FingerprintDiffer #219

wants to merge 1 commit into from

Conversation

JeroenDeDauw
Copy link
Contributor

For optimization in https://phabricator.wikimedia.org/T220150

This is a re-submit of wmde/doctrine-term-store#9

Copy link
Member

@lucaswerkmeister lucaswerkmeister left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This has an extremely confusing name. Under FingerprintDiffer, I expect a class that can be used by ItemDiffer and PropertyDiffer (partially replacing their current toDiffArray methods, just like they already use a StatementListDiffer to diff claims), accepting two Fingerprints and returning some subclass of Diff, which can then be passed into FingerprintPatcher (which exists!). As far as I can tell, this implements something very different and not as generally useful (due to the asymmetry between $first and $second). I’m not sure whether that more specific class should be in data-model-services (it sounds like it’s fairly specific to the term store use-case), but I’m certain that it should not be called a FingerprintDiffer.

@JeroenDeDauw
Copy link
Contributor Author

This class follows the array_diff pattern. What would you call it?

Also, we have one use case where this is better for the consumer than using the Diff library and zero use cases where the opposite is true.

@lucaswerkmeister
Copy link
Member

In Add SchemaPatcher class, I used a FingerprintPatcher; if a FingerprintDiffer existed, I’m sure I would have tried to use it in Add SchemaDiffer class as well, and gotten frustrated when it doesn’t match the rest of the library at all.

Ideas for alternative names: something with “fingerprint removal”, or “complement” instead of “difference”.

@JeroenDeDauw
Copy link
Contributor Author

The current name is consistent with array_diff and the other diff functions in PHP. You're right that it uses a different approach then the Diff library based code. Not everything needs to follow the same approach.

For the use case you linked it might well have been good to have something based on Diff, though that is not the use case this code is written for. If you get frustrated with some code written for a different use case not serving your use case, that is not a problem with the code.

I cannot think of a more sensible name than the one used here and no concrete alternative has been suggested so far.

@JeroenDeDauw JeroenDeDauw requested a review from AlaaSarhan May 15, 2019 19:11
@mariushoch
Copy link
Member

Maybe name this DifferenceFingerprint… (where something like Service, or Computer, …) given that this outputs the difference of two Fingerprints in a Fingerprint?

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

Successfully merging this pull request may close these issues.

3 participants