Skip to content

Conversation

@horpto
Copy link

@horpto horpto commented Mar 27, 2023

Hello, we use moq to generate mocks in the different directory in 1 mock to 1 file. So our genmock.go has a pile lines like this:

//go:generate moq -out "mocks/fee.go" -fmt goimports -skip-ensure . IFeeInterface
//go:generate moq -out "mocks/foo.go" -fmt goimports -skip-ensure . IFooInterface
//go:generate moq -out "mocks/goo.go" -fmt goimports -skip-ensure . IGooInterface

I'd like to add a feature to just specify directory and list of interfaces in one package. This also should be more quickly to generate mocks.

Also I fix version indication in release as gorelease will specify main.version, not main.Version

Copy link
Contributor

@breml breml left a comment

Choose a reason for hiding this comment

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

Hi @horpto

Thanks for your contribution. I skimmed through your PR and some thoughts come to mind, that I would like to share with you.

General:

  • For such an extension, it would have been beneficial to first create an issue, such that the topic can be discussed and multiple different approached could have been evaluated.
  • Maybe the fix for the version should be in a separate PR, since this is pretty likely to be accepted and merged quickly, instead of combining this with a PR, where there is not yet any guarantee, that it will get accepted in due time.

Specific for the PR:

  • The PR contains special handling for interface names, starting with I. To my knowledge, this is a pattern, that is not normally used in Go code (I would even go so far, that it is discouraged) and therefore I wonder, if a tool like moq should provide support for this.
  • For this PR to be merged, the README.md should be updated as well and reflect the changes to the flags and the arguments. Specifically it should mention, which flags are mutual exclusive.


func main() {
var flags userFlags
flag.StringVar(&flags.outFile, "out", "", "output file (default stdout)")
Copy link
Contributor

Choose a reason for hiding this comment

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

Should the -out flag mention as well, that it is exclusive with -out-dir?

main.go Outdated
if err = m.Mock(out, args...); err != nil {
switch {
case flags.outDir != "" && flags.outFile != "":
return errors.New("use only one from -out and -out-dir arguments")
Copy link
Contributor

Choose a reason for hiding this comment

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

Suggested change
return errors.New("use only one from -out and -out-dir arguments")
return errors.New("use only one of the flags -out and -out-dir")

@horpto
Copy link
Author

horpto commented Mar 28, 2023

Hi @breml! Ok, I created a new pr with only small changes.

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