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

Fixed -mockNames not propagating to Recorder and ReturnCall types #127

Merged
merged 3 commits into from
Dec 19, 2023

Conversation

UnAfraid
Copy link
Contributor

@UnAfraid UnAfraid commented Dec 13, 2023

When you are trying to mock multiple interfaces with the same name and similar methods (for example user.Service Create, post.Service Create, etc..) there is collision in Recorder and ReturnCall types, the -mockNames parameter only changes the main interface name, with the changes proposed in this PR the correct mockName will be applied to the Recorder and ReturnCall types.

Addresses #117

@r-hang r-hang self-requested a review December 18, 2023 18:56
Copy link
Contributor

@r-hang r-hang left a comment

Choose a reason for hiding this comment

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

The idea make sense to me. However, I think there are advantages to keeping *model.Interface plumbed through the calls to GenerateMockRecorderMethod and GenerateMockReturnCall method in case we need the richer representation in future code generation changes. Where we use intf.Name could we just use g.MockName(int.Name) instead?

Could we also add a test for this scenario under mockgen/internal/tests?

@UnAfraid
Copy link
Contributor Author

UnAfraid commented Dec 18, 2023

The idea make sense to me. However, I think there are advantages to keeping *model.Interface plumbed through the calls to GenerateMockRecorderMethod and GenerateMockReturnCall method in case we need the richer representation in future code generation changes. Where we use intf.Name could we just use g.MockName(int.Name) instead?

Sure thing, i'll bring back the parameter *model.Interface.

Could we also add a test for this scenario under mockgen/internal/tests?

Of course

@UnAfraid UnAfraid force-pushed the feature/mock_names_propagate branch from 7d44f56 to 196a352 Compare December 18, 2023 23:06
@UnAfraid UnAfraid force-pushed the feature/mock_names_propagate branch 2 times, most recently from 0c6bbe9 to 66690a2 Compare December 18, 2023 23:36
@UnAfraid UnAfraid force-pushed the feature/mock_names_propagate branch from 66690a2 to 753b99b Compare December 18, 2023 23:37
@r-hang r-hang merged commit 6dd8fe5 into uber-go:main Dec 19, 2023
3 checks passed
@UnAfraid UnAfraid deleted the feature/mock_names_propagate branch December 20, 2023 12:17
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.

2 participants