-
Notifications
You must be signed in to change notification settings - Fork 121
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
feat: add Cond matcher #60
feat: add Cond matcher #60
Conversation
Fn matcher allows to use custom logic for complex matching scenarios
@moisesvega @JacobOaks @tchung1118 any feedback (especially on the exposed function name) are more than welcome. |
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.
@damianopetrungaro @sywhang @tchung1118 what are your thoughts on this vs. just defining your own type that implements Matcher?
I think this could be useful to have when you need to write a lot of different sets of custom matching logic "on-the-fly", but I'm not sure if it's too much to have this level of generalization and the generalization of being able to implement Matcher
through a custom type.
gomock/matchers.go
Outdated
@@ -97,6 +97,18 @@ func (anyMatcher) String() string { | |||
return "is anything" | |||
} | |||
|
|||
type fnMatcher struct { |
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.
What are your thoughts on making this type just a redefinition of the func type since it doesn't really need to be a struct?
type fnMatcher fn(any) bool
func (f fnMatcher) Matches(x any) bool {
return f(x)
}
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 am cool with both as I don't see this as a red flag.
I based this implementation on the fact that most of the currently implemented Matcher
are structs with one or no fields at all, reducing the implementation difference as much as possible.
@JacobOaks, this is not a mutually exclusive option; this can be the building block for the logic you are referring to. As an example, you can leverage the custom function as the building block for the logic you're suggesting: var DefaultUserMatcher = gomock.Fn(func(x any)bool{ _logic here_ }) as well as using it on the fly for complex and dynamic matching occurrences. |
I think this could be a good convenience feature for this library. |
Hey @damianopetrungaro, I think this makes sense to come into the library. Let's do a different name though. Maybe something like |
749161e
to
6161abd
Compare
@JacobOaks makes sense to me! |
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.
Just one nit about the comment. Otherwise, LGTM!
@tchung1118 can you take a quick look too?
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.
LGTM, but I'd suggest updating the PR message before merging.
@tchung1118 updated the PR message, for the commit message if we squash and merge we can do that via the UI, if not I can rebase and push force the commit with a different message as well, up to you. |
You don't need to rewrite any commit message. When you squash and merge, PR message becomes the commit message of the merge commit by default, so I just wanted the PR message to be the final commit message. |
Cool to be merged then @tchung1118 |
Cond
is a matcher allowing to rely on custom logic for complex matching scenarios.It happened quite a few times that I had to leverage on the
DoAndReturn
to perform complex matching use cases, passinggomock.Any()
as a matcher.This would have allowed me to reduce a lot of complexity.