Skip to content

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