Conversation
There was a problem hiding this comment.
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.
|
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. |
|
In Add Ideas for alternative names: something with “fingerprint removal”, or “complement” instead of “difference”. |
|
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. |
|
Maybe name this |
For optimization in https://phabricator.wikimedia.org/T220150
This is a re-submit of wmde/doctrine-term-store#9