Repository navigation
DelayedConstraint shows full result of underlying constraint. - #4922
Conversation
ed559fa to
0071137
Compare
stevenaw
left a comment
stevenaw
left a comment
There was a problem hiding this comment.
Thanks for the PR @manfred-brands
I hadn't time to properly review tonight, but I suspect you may've fixed a third bug too here 🙂
| public override void WriteMessageTo(MessageWriter writer) | ||
| { | ||
| writer.WriteMessageLine(Description); | ||
| _innerResult.WriteMessageTo(writer); |
There was a problem hiding this comment.
Thanks for putting this PR up @manfred-brands
I haven't tested this myself, but I suspect this may also fix #4545
Not that I am suggesting we explicitly pursue it in this PR if it doesn't, merely that we may get a third bug fix "for free" from your changes
There was a problem hiding this comment.
You are good at finding issues. Unknowingly I followed a similar solution as described in that ticket.
stevenaw
left a comment
stevenaw
left a comment
There was a problem hiding this comment.
Thanks @manfred-brands ! This looks great to me too.
I left one minor suggestion to make the new MsgUtils method private unless there was a use case you knew of to keep it public
| /// </summary> | ||
| /// <param name="val">The object to format.</param> | ||
| /// <returns>Formatted string for all properties.</returns> | ||
| public static string FormatValueProperties(object? val) |
There was a problem hiding this comment.
nitpick: Was there a use case for making this public, or could it be made private to maximize our ability to evolve it without breaking others?
For example, there could be a time in the future where we'd want to pass in a precomputed collection of properties from elsewhere in the framework, or perhaps allowing this to recurse to child objects with a configurable "depth".
There was a problem hiding this comment.
nitpick: Was there a use case for making this public, or could it be made private to maximize our ability to evolve it without breaking others?
It cannot be private as it is called from another class (EqualConstraintResult). I didn't want to introduce it in the formatter call chain for all values as that will have side effects where properties are not to be printed, like DbContext.
The whole class MsgUtils is internal and therefore we can change the signature without breaking external code.
There was a problem hiding this comment.
Oh! I had meant internal, but has missed that MsgUtils itself was already internal. Sorry about that
aafc1ca to
8b91299
Compare
3 participants
Fixes #4909
Fixes #4919
Fixes #4545