Merged
Conversation
|
|
8ff0dc9 to
9ec4913
Compare
Contributor
|
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:
PS: I'm not an admin who can decide whether to merge this PR or not. |
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 |
Contributor
Author
|
Thanks both for your comments. I agree @favonia's suggestions are a nice addition. Have pushed up a new commit fyi @JacobOaks |
favonia
reviewed
Oct 29, 2023
favonia
reviewed
Oct 29, 2023
favonia
reviewed
Oct 29, 2023
favonia
reviewed
Oct 29, 2023
Contributor
|
@merrett010 I made three suggestions:
I still remain neutral about whether this feature should be included. |
Contributor
Author
|
@favonia Thanks for reviewing, all seemed like good suggestions to me |
JacobOaks
reviewed
Oct 30, 2023
Contributor
JacobOaks
left a comment
There was a problem hiding this comment.
Mostly looks good to me - just two couple small suggestions.
Contributor
Author
|
Cheers @JacobOaks, have seen to those now |
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
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
4 participants
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Closes #113
Let me know if it's something you'd be interested in having - happy to make some tweaks if required