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

proposal: Add OneOf Matcher #91

Closed
wants to merge 1 commit into from

Conversation

abshierjoel
Copy link

This is one potential implementation of proposal #90

This proposal introduces a new Matcher to gomock for matching a parameter against is list of potential values it may have. This should help ease-of-use, when multiple matches are necessary and additionally help navigate around an existing issue with overriding matches.

I would appreciate any and all feedback on this proposal! Thank you!

@CLAassistant
Copy link

CLAassistant commented Sep 16, 2023

CLA assistant check
All committers have signed the CLA.

//
// OneOf([]any{100,200,300})
// OneOf([]any{"Go", "Gopher"})
func OneOf(x []any) Matcher {
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Maybe we can use ... instead of slice?
func OneOf(x any...) Matcher

Copy link
Contributor

@favonia favonia Sep 16, 2023

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@tulzke I personally agree, and I did that in #63. 😉

@favonia
Copy link
Contributor

favonia commented Sep 16, 2023

Hi, I believe this PR is largely subsumed by my PR #63, modulo the difference in the interface (the name OneOf versus AnyOf and whether it is taking a slice versus a variable number of arguments). The difference seems minor because one can write AnyOf(choices..) for a slice choices. On the other hand, the AnyOf in #63 can also take matchers such as Len(100), which I believe is more powerful than the OneOf in this PR.

@favonia
Copy link
Contributor

favonia commented Sep 16, 2023

I want to add that this feature has been discussed many times, even before the transfer to Uber. Here were some related discussions (including the one after the transfer):

I hope that the PR #63 (or at least this PR) can be merged soon. 😀

@r-hang
Copy link
Contributor

r-hang commented Sep 18, 2023

As an update here, I've already approved and plan to merge #63 (review) to address this issue.

@r-hang
Copy link
Contributor

r-hang commented Sep 19, 2023

@abshierjoel, given #63 is it okay if we close this issue?

@r-hang
Copy link
Contributor

r-hang commented Sep 26, 2023

Closing now that #63 has landed.

@r-hang r-hang closed this Sep 26, 2023
@abshierjoel
Copy link
Author

@r-hang apologies for the slow response on this. Thanks for feedback @tulzke and @favonia -- I really appreciate it! Glad we'll got something better with #63!

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.

5 participants