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

Allow constructing recursive MapDiffer that uses itself #103

Open
wants to merge 2 commits into
base: master
Choose a base branch
from

Conversation

JeroenDeDauw
Copy link
Collaborator

@JeroenDeDauw JeroenDeDauw commented Apr 24, 2018

Fixes #98

@JeroenDeDauw JeroenDeDauw mentioned this pull request Apr 24, 2018
@JeroenDeDauw
Copy link
Collaborator Author

JeroenDeDauw commented Apr 24, 2018

@BoShurik I made a follow up, now it should be ready for merging.

Really a hacky solution though. Making this follow up made me realize how strange it is that the Differ return an array of DiffOp rather than a Diff. I don't see any reason for that. If they returned a Diff, we'd not have the problem in MapDiffer, that it needs to know if the inner differ is associative or not.

I'm thinking of just creating a version 4.0 that

  • Ditches Diff in favor of new ListDiff and MapDiff that both extend a common Diff interface
  • ListDiffer interface that returns ListDiff
  • MapDiffer interface that returns MapDiff
  • Add isAssociative to DiffOp, maybe removing isAtomic
    If that is done, then perhaps adding the MapDifferInterface now is a bit silly.

So I am not comfortable with merging this without review from someone else that thinks this is OK.

@JeroenDeDauw
Copy link
Collaborator Author

Another non-big-changes approach that avoids introducing MapDifferInterface is to add a new constructor argument $recursiveMapDiffs that would then replace the instanceof check in the current PR.

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.

2 participants