Add support to dump superclass nodes #58
Conversation
|
@stephencelis This is a continuation of pointfreeco/swift-snapshot-testing#597 here because I was setting up a plugin (https://github.com/tahirmt/swift-snapshot-testing-dump) to use this library for the dump but these features are still good to include since I want to have control over what gets dumped. |
|
@tahirmt Thanks for moving things here, and for the issue! I think we should definitely include superclass properties by default in our dump. And folks that don't want them can use a custom mirror using the existing protocols. Because of this I think we could simplify the implementation. Can we avoid the new protocols that you introduced for configuration and instead always use the superclass mirror? Do you foresee any issues with such a change? |
|
@stephencelis I was noticing issues with system classes like |
|
Interesting. For things like |
|
Certainly There's overall 25 failures. I agree we could provide an internal way of not logging superclass for these and provide an inverse protocol to hide superclass mirror someone wants to do it |
|
@tahirmt If you push a failing branch I'd love to take a look at the actual dump output to get a feel. |
Sources/CustomDump/Dump.swift
Outdated
| out.write("\(value)") | ||
| } | ||
|
|
||
| if let superclassMirror = mirror.superclassMirror, value is CustomDumpIncludeSuperclass { |
There was a problem hiding this comment.
If you remove this value is CustomDumpIncludeSuperclass check the failures will happen
There was a problem hiding this comment.
tahirmt
force-pushed
the
dump-improvements
branch
from
1e9be6b to
cfa341e
Compare
tahirmt
force-pushed
the
dump-improvements
branch
from
cfa341e to
a799dad
Compare
|
@stephencelis This reminded me of a possible bug I might have mentioned to you a while back. The listed items would prevent items from dumping even if if they weren't recursive. |
|
@iampatbrown what issue is there on your branch? Your solution seems to be nicer than mine actually in that it combines the superclass properties with the subclass properties in the dump |
|
Ohh I think I see now. You're talking about a scenario where the object is repeated but there is no recursion it still skips the object from the dump because it was already dumped. I still feel it doesn't really add value to dump it over and over but perhaps adding a way to identify which one it is because there could be multiple instances of the same type where only one is repeated. What if we assign each instance of any class a unique identifier, e.g. class User {
let name: String
init(name: String) { self.name = name }
}
let user = User(name: "James")
let user2 = User(name: "John")
dump = ""
customDump([
user,
user,
user2,
user2
], to: &dump)This will dump So perhaps we can assign an identifier to it instead so it prints something like this And by not assigning a number to the first instance we will make sure most of the dumps won't include identifiers. |
Yep. That's it. I'm unsure if it's the desired behaviour or not. But it was problematic in my case. It might be worth leveraging Mirror.AncestorRepresentation which could be used with |
|
@iampatbrown I've never used Curious what @stephencelis has to say about adding a way to identify the repeated objects in the dump without repeated dumps because that could end up being a lot of extra lines? |
tahirmt
changed the title
|
I have remove the additional protocols from this PR and only left the superclass properties in this PR for now. |
|
@iampatbrown implemented my idea about assigning ids to track repeat occurrences #60 |
|
@stephencelis is there anything else I can do to get this moved forward? |
|
Hi @tahirmt! Sorry for the delay! Just haven’t had time to catch up on this. Will take a look later today. |
|
@tahirmt Thanks for providing this much simpler implementation! I think it looks good, but will chat with Brandon about it on Monday. |
Adding support for the following things
Closes #57