Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
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
Correct highlighting for asymmetric matchers #7893
Correct highlighting for asymmetric matchers #7893
Changes from all commits
59937bc
e26d9cd
fb70d79
ea15b24
91c8b7c
9308f69
5b1daef
391fbfa
16d7243
File filter
Filter by extension
Conversations
Jump to
There are no files selected for viewing
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.
Yeah, when reading this example (in particular the
{b: 1}
part at the end) I already kind of saw the question down there coming 😅To express what I understand in my words and make sure we're on the same page:
The original
DiffObject
is a tree structure, while thisLine
does not have furtherLine
s in itsval
- it is flat. So theseLine
s can only represent the leaves of theDiffObject
tree, meaning in your example the outer{a: ...}
(which is UPDATED, not a leaf) is handled differently from the inner{a: ...}
(which is UNEQUAL_TYPE, a leaf).At the risk of suggesting something you had already thought about and know is problematic:
Flattening the tree seems to be the problem - I think we want to traverse a full
DiffObject
tree that goes all the way down into all nested structures.Does diffing
{}
vs{a: {b: 1}}
result in tree 1 (from what I understand it currently does):or tree 2:
If we have tree 2 (we still go all the way into an
INSERTED
property, it's just that every child will necessarily also beINSERTED
), then the printer can do a recursive traversal pass through the tree, increasing indentation whenever it descends into child diffs, and really all that theINSERTED
/DELETED
kinds do is modify the sign that the printer prints at the start of the line.EQUAL
vsUPDATED
would be almost the same for the printer really, only difference I can imagine is that the printer must never abbreviate anUPDATED
node, it always has to print it out to get to the actual differences inside.UNEQUAL_TYPE
is a bit awkward, I think it would have to go and instead be a pair ofDELETED
andINSERTED
, because what kind would the child diffs of anUNEQUAL_TYPE
be?Let me know what you think of this approach. I think so far the assumption of "when a new property appears (or a property has a different type), mark the whole thing different in some form and move on" has not really been challenged, but it seems implementing a printer has shown that it would be beneficial to have the WHOLE structure already analyzed so that the printer can just traverse through it - otherwise this problem occurs where
UPDATED
objects are treated as transparent by the printer and recursed into, whileINSERTED
objects are treated as opaque and just printed out as one big bag of characters, necessarily using different code for the two.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.
It's an interesting approach. I am investigating these days whenever I have time.
From a high-level view, this approach would change the responsibilities of
DiffObject
, but probably it will help out formatter. I liked thatDiffObject
had no idea about the formatting part. It's a bit unclear to me what's the responsibility ofDiffObject
is now(I get it from implementation point of view).From the implementation view, all nested structure diff functions require two values.
diffObjects(a, b, opts, diff)
and the same in formatter. I am working to changing things up. It's not that simple. A lot of things and types need to be changed around. I will update here if I have new insights. Currently, I am thinking that the plugin system could be a bit harder to implement.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.
I think the responsibility is still fairly clear, it marks the parts that were different with
kind
s. It's true that there would now be lots of subtrees that are entirely equal. I view it as:Before, the maximum tree depth we would store for equal parts of the objects is 1.
Now, the maximum tree depth we would store for equal parts of the objects is
POSITIVE_INFINITY
.The equal parts were always there, it's just we didn't bother to store deeper levels of them. We were going through them anyway when figuring out that they were actually equal at the start. But now we store them entirely, because it helps consumers of the diff object, like the printer.
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.
Some updates:
getObjectKeys
gets reused. I was thinking to use the idea of empty structures({}
for objects and[]
for arrays) and diff against them in a or b position to get flattened inserted/deleted diffs. But not sure if there can be an empty structure for everything, mostly asymmetric matchers worried me. Might get back to that idea at some point.diffWithCustomDiffs
is created in the main diff function with plugins and then passed down. The same would need to happen with flatten function. Those two points combined, I think that maybe it's best to add flatten as a separate step? It's an extra traversal but it's a bit awkward to carry both flatten and diff functions around.So it with extra step it would be diff -> flatten -> format -> print. Not sure if it's worth it, but something I would be willing to try.
In the future, formatter might just show the name of the constructor only when types are unequal or be smart about it in other ways.
expect.any(Object)
and{ a: 1 }
looks like? Because they are equal, but we won't have flattened object for it. So far I am doing this, but the assumption that EQUAL means two objects have the same structure and will get flattened during the diffing process does not always stand.Meanwhile, I will work to clean up the code(mostly types) and push it. But hopefully, this is enough to get the idea of some problems.
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.
Hmmm, I'm not sure I follow. You are raising problems that occur when trying to flatten a diff object tree, but what I was trying to say is I would want to avoid even doing a flatten at all costs.
In my suggestion, the diff object tree would always have as much depth as the values we're comparing; it would include all parts of a and b, where a and b are equal, where a has a property that b doesn't have at all, and in any other case.
That tree would never be flattened, instead the printer would operate by traversing into the full-depth structure.
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.
There are couple of problems with that:
First, We cannot use
undefined
because it will treat missing key and key which has value ofundefined
the same way.Second, I tried the same approach but with different special type, but it just passed down the problem(and functions) to the next guy and makes code very hard to follow. In general diffing algorithms and traversing algorithms are quite different in our case and there is very little elegant reuse that can be achieved. I feel like I tried several approaches already and always hit the dead end.
For now it's not a problem, it might be but we can worry about it later. None of this traversal changes are catastrophic and can always be reverted.
I will try to push code ASAP now so you also can get clearer idea
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.
Hmm, couldn't it still be represented differently to make the strict equality
undefined
vs missing property that you mention work?Consider the following pseudocode (of course simplified without things like
path
, also ignoring the other way around withkeysOnlyInA
andDELETED
, ...) - what am I missing?This is a really interesting conversation by the way 👌 in general assertions and diffing are such an interesting topic 😅
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.
I agree it's very interesting and it's great to hear your thought process about it too.
I have been working on this whenever I had time. Pushing the newest changes so you can see them, but it's not final work.
This version is recursively traversing all complex objects and marks children as inserted/deleted.
Instead of
undefined
I went with Symbol which marks an empty value. You can see the code incomplex/object.ts
indiff
function.Additionally improved typing and rebased, which was some work considering how much changed.
I am planning to work on it more actively this week.
First I will try to make CI happy and then go through skipped tests(which are mostly for missing features) and try to make them pass.
Do you have any remarks/questions regarding overall architecture?
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 union type does look cool indeed 👍
I would say overall architecture is mostly comprised of how
diff
traverses through data to build the diff object tree, and howformat
traverses through the diff object tree to print it, so I think we're aligned at this point 😅Did you, while writing the code, feel like the approach I pushed for (that relies more heavily on tree-traversal) was helping against code duplication and feeling more natural? Or were there still awkward bits?
That would be interesting to me because of course my suggestions were all coming from quite a high level and I can't know for sure if they still work well when getting into the details implementing it
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.
I have tried and it has some quirks. Not saying no to it, but I wanted to implement some other types and see some diffs first. Since diff function API is mostly agreed on, how we implement it can be improved down the line.