Skip to content
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

Merged
merged 4 commits into from
Aug 16, 2023

Conversation

damianopetrungaro
Copy link
Contributor

@damianopetrungaro damianopetrungaro commented Aug 12, 2023

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, passing gomock.Any() as a matcher.

This would have allowed me to reduce a lot of complexity.

Fn matcher allows to use custom logic for complex matching scenarios
@damianopetrungaro
Copy link
Contributor Author

@moisesvega @JacobOaks @tchung1118 any feedback (especially on the exposed function name) are more than welcome.

Copy link
Contributor

@JacobOaks JacobOaks left a 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.

@@ -97,6 +97,18 @@ func (anyMatcher) String() string {
return "is anything"
}

type fnMatcher struct {
Copy link
Contributor

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)
}

Copy link
Contributor Author

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.

gomock/matchers.go Outdated Show resolved Hide resolved
@damianopetrungaro
Copy link
Contributor Author

damianopetrungaro commented Aug 14, 2023

@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.

@tchung1118
Copy link
Contributor

I think this could be a good convenience feature for this library.

@JacobOaks
Copy link
Contributor

Hey @damianopetrungaro, I think this makes sense to come into the library. Let's do a different name though. Maybe something like Cond or CondFn.

@damianopetrungaro
Copy link
Contributor Author

@JacobOaks makes sense to me!
Pushed 😄

@damianopetrungaro damianopetrungaro changed the title feat: add Fn matcher feat: add Cond matcher Aug 15, 2023
Copy link
Contributor

@JacobOaks JacobOaks left a 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?

gomock/matchers.go Outdated Show resolved Hide resolved
Copy link
Contributor

@tchung1118 tchung1118 left a 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.

@damianopetrungaro
Copy link
Contributor Author

@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.

@tchung1118
Copy link
Contributor

@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.

@damianopetrungaro
Copy link
Contributor Author

Cool to be merged then @tchung1118

@JacobOaks JacobOaks merged commit 665220d into uber-go:main Aug 16, 2023
2 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants