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 Regex() matcher #114

Merged
merged 4 commits into from
Oct 30, 2023
Merged

feat: add Regex() matcher #114

merged 4 commits into from
Oct 30, 2023

Conversation

merrett010
Copy link
Contributor

Closes #113

Let me know if it's something you'd be interested in having - happy to make some tweaks if required

@CLAassistant
Copy link

CLAassistant commented Oct 16, 2023

CLA assistant check
All committers have signed the CLA.

@favonia
Copy link
Contributor

favonia commented Oct 19, 2023

Hi, I found this PR interesting---though I'm neutral about whether this feature should be added. If it's to be added, I wonder if we could make these two changes:

  1. Immediately compile the regular expression into a regexp.Regexp instead of compiling it during every check.
  2. Match []byte in addition to string. Maybe even io.RuneReader.

PS: I'm not an admin who can decide whether to merge this PR or not.

@JacobOaks
Copy link
Contributor

Hi @merrett010 thanks for this PR. I'm not opposed to adding this feature if others aren't.

However, I agree with both of @favonia's suggestions. While in most cases I would imagine the matcher will be used a single time - it wouldn't hurt to be on the safe side and compile the regular expression once. Matching []byte would also be a nice touch.

@merrett010
Copy link
Contributor Author

Thanks both for your comments. I agree @favonia's suggestions are a nice addition. Have pushed up a new commit fyi @JacobOaks

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

favonia commented Oct 29, 2023

@merrett010 I made three suggestions:

  1. Panic early when the the regular expression is invalid instead of silently ignore it.
  2. Use character literals instead of character codes.
  3. Print "matches ..." instead of "matching ...".

I still remain neutral about whether this feature should be included.

@merrett010
Copy link
Contributor Author

@favonia Thanks for reviewing, all seemed like good suggestions to me

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.

Mostly looks good to me - just two couple small suggestions.

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

Cheers @JacobOaks, have seen to those now

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.

LGTM!

@JacobOaks JacobOaks merged commit 94a7ac3 into uber-go:main Oct 30, 2023
3 checks passed
JacobOaks added a commit to JacobOaks/mock that referenced this pull request Sep 18, 2024
A deadlock related to controller calling Stringer on mocks themselves
was revealed in uber-go#116. uber-go#114 solved this deadlock by adding a generated
`ISGOMOCK()` method to all generated mocks, and then checking for it
before calling `.String()` on arguments.

This reveals an exported method on each generated mock that:
* Bloats the generated code
* Can be taken dependency on in strange ways via Hyrum's Law
* Technically opens up another route for naming collision.

This PR attempts to clean up this type of check by instead generating
an unexported field in generated mock structs instead, and checks for it using reflect.
This hides this implementation detail of gomock/mockgen better,
and produces less generated bloat.

Ref: uber-go#116
JacobOaks added a commit to JacobOaks/mock that referenced this pull request Sep 18, 2024
A deadlock related to controller calling Stringer on mocks themselves
was revealed in uber-go#116. uber-go#114 solved this deadlock by adding a generated
`ISGOMOCK()` method to all generated mocks, and then checking for it
before calling `.String()` on arguments.

This reveals an exported method on each generated mock that:
* Bloats the generated code
* Can be taken dependency on in strange ways via Hyrum's Law
* Technically opens up another route for naming collision.

This PR attempts to clean up this type of check by instead generating
an unexported field in generated mock structs instead, and checks for it using reflect.
This hides this implementation detail of gomock/mockgen better,
and produces less generated bloat.

Ref: uber-go#116
JacobOaks added a commit to JacobOaks/mock that referenced this pull request Sep 18, 2024
A deadlock related to controller calling Stringer on mocks themselves
was revealed in uber-go#116. uber-go#114 solved this deadlock by adding a generated
`ISGOMOCK()` method to all generated mocks, and then checking for it
before calling `.String()` on arguments.

This reveals an exported method on each generated mock that:
* Bloats the generated code
* Can be taken dependency on in strange ways via Hyrum's Law
* Technically opens up another route for naming collision.

This PR attempts to clean up this type of check by instead generating
an unexported field in generated mock structs instead, and checks for it using reflect.
This hides this implementation detail of gomock/mockgen better,
and produces less generated bloat.

This PR then regenerated all generated code for tests/example via `go generate`.

Ref: uber-go#116
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.

Add a matcher for checking a string parameter against regex
4 participants