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

Extend gomock to allow passing an source_importpath instead of library when operating in source mode #3350

Open
wants to merge 1 commit into
base: master
Choose a base branch
from

Conversation

ramenjosh
Copy link
Contributor

What type of PR is this?

Feature

What does this PR do? Why is it needed?

This PR adds a new parameter, importpath to the gomock rule. This parameter is optional, and allows a gomock target running in source mode to avoid depending on a go_library. As far as I can see, the only use for the go_library in source mode is to extract an importpath.

I've also extended the test cases to test the different modes gomock can operate in, and made sure there's a link to gomock on the top level README.

Which issues(s) does this PR fix?

Fixes #3349

Copy link
Member

@fmeum fmeum left a comment

Choose a reason for hiding this comment

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

This looks very reasonable to me - it's a very simple way to get by without a potentially annoying dependency edge.

extras/gomock.bzl Show resolved Hide resolved
mandatory = True,
mandatory = False,
),
"importpath": attr.string(
Copy link
Contributor

Choose a reason for hiding this comment

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

I am afraid the attribute name importpath would confuse Gazelle: now there are two targets with the same importpath value. How about calling it source_package?

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 think calling it source_package would cause confusion with package, maybe source_importpath would be a good middle ground?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

There's actually also self_package, which has this description:

self_package: the full package import path for the generated code. The purpose of this flag is to prevent import cycles in the generated code by trying to include its own package. See [mockgen's -self_package](https://github.com/golang/mock#flags) for more information.

Perhaps we can avoid introducing a new parameter and just use this one?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Hmm, on second thought it's probably better to avoid overloading the parameter given it has special meaning to mockgen itself. Have gone with source_importpath for now.

@ramenjosh ramenjosh requested review from linzhp and fmeum and removed request for linzhp and fmeum November 17, 2022 00:07
@ramenjosh
Copy link
Contributor Author

@fmeum @linzhp can you please take another review? There's still one part of the CI that seems to be failing and I'm not super sure why, any tips on what might be causing it?

@ramenjosh ramenjosh requested review from fmeum and linzhp and removed request for linzhp and fmeum November 17, 2022 00:08
@fmeum
Copy link
Member

fmeum commented Nov 17, 2022

@ramenjosh To be honest, I have no idea what is causing that failure. Could you amend and force-push your branch to trigger a new CI run? It may just be a flake.

…rary` when operating in source mode. This enables a use case where mocks can be included as part of the source files in a go_library. Added additional test cases and documentation.
@ramenjosh ramenjosh force-pushed the jsmith-gomock-extension branch from 6e67e3f to 37dddb3 Compare November 17, 2022 02:32
@ramenjosh ramenjosh changed the title Extend gomock to allow passing an importpath instead of library when operating in source mode Extend gomock to allow passing an source_importpath instead of library when operating in source mode Nov 17, 2022
@therve
Copy link
Contributor

therve commented Jan 9, 2024

@fmeum Can we revive that change? The issue is still present. Thanks.

@fmeum
Copy link
Member

fmeum commented Jan 9, 2024

Sure, I can review this if someone can resolve the conflicts. The flaky tests should be gone now.

@therve
Copy link
Contributor

therve commented Jan 9, 2024

Thanks, created #3822.

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.

Circular dependencies when including mocks generated by gomock in go_library
4 participants