Feature/749: Receive(POINTER,MATCHER) #750
Conversation
Signed-off-by: thediveo <thediveo@gmx.eu>
Signed-off-by: thediveo <thediveo@gmx.eu>
|
This looks great to me! |
| } | ||
| if len(args) > 0 { | ||
| arg := args[0] | ||
| subMatcher, hasSubMatcher = arg.(omegaMatcher) |
There was a problem hiding this comment.
I think this was a breaking change, previously we checked if matcher.Arg != nil before trying to convert it to omegaMatcher.
There was a problem hiding this comment.
The previous implementation took the first arg, if there are one or more args, silently ignoring additional unused args.
In turn, if there was a at least a first arg (matcher.Arg != nil) it differentiated between POINTER or MATCHER. This user-facing behavior is preserved, the corresponding unit tests are unmodified in this respect. The new implementation preserves this behavior, but choose not to implement this as three different cases, but instead to use a stepwise approach, removing matching POINTER/MATCHER args, while enforcing the correct order in case both POINTER and MATCHER appear.
The change now accepts up to two optional args, where in case of two args the order must be POINTER, then MATCHER.
This should be only a breaking change to unit tests that in the past accidentally passed two or more args, with all args except for the first being silently ignored. This undefined behavior is now caught and reported.
Is there a different breaking change that I'm missing here?
There was a problem hiding this comment.
On second thought, maybe Receive was being used incorrectly but calling Receive(nil) with nil as the first argument worked before and it threw an error on upgrading. It's more explicit now anyway though, so not actually a problem.
There was a problem hiding this comment.
Ah, I wasn't aware of the explicit nil usage.
There was a problem hiding this comment.
ah i see. if anything, I would say this was a bug that is now fixed. Gomega should have been nudging folks to use BeNil() in this case as nil can be ambiguous.
Based on discussion #749:
ReceiveMatcherto handle a new variantReceive(&actual, Matcher("foo"))