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

gomock_generator. Add --source-path option #930

Open
wants to merge 2 commits into
base: master
Choose a base branch
from

Conversation

sc-zenokerr
Copy link
Contributor

  • Add a changelog entry in CHANGELOG.md

Add a --source-path option that allows for the user to specify the location of their codebase.

e.g. gomocks_generator --source-path <path_to_codebase>

Copy link
Member

@EtienneM EtienneM left a comment

Choose a reason for hiding this comment

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

I'm not sure I understand the need for this option. Is it to be able to execute gomock_generator from another folder than the service root folder?

@sc-zenokerr
Copy link
Contributor Author

Hi @EtienneM .

The reason is that the local version of the codebase might not be in the default location.

Prior to this change, a codebase such as Scalingo/go-utils would be located $GOPATH/src/github.com/Scalingo/swan-agent

gomock_generator REQUIRES the codebase to be located in the default location.

The --source-path option allows us to locate the codebase somewhere other than the default location.

Comment on lines 31 to 32
// SourcePath override the output base path of the mocks
SourcePath string
Copy link
Member

Choose a reason for hiding this comment

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

issue: I'm not sure I understand the choice of words here for the variable name. It mentions Source but it seems to be the location of what this program outputs. What do you think about MocksPath or MocksRootPath?

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'm not precious about the naming... but it is the location of your codebase. MocksRootPath works. I'll update it.

Comment on lines 89 to 92
mockSigsPath := path.Join(gcfg.SourcePath, mocksCfg.BaseDirectory, gcfg.SignaturesFilename)
if gcfg.SourcePath != "" {
mockSigsPath = path.Join(gcfg.SourcePath, gcfg.SignaturesFilename)
}
Copy link
Member

Choose a reason for hiding this comment

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

issue: from what I understand, it shouldn't be needed at this point to still have a condition. mocksPath contains the value we need.

Suggested change
mockSigsPath := path.Join(gcfg.SourcePath, mocksCfg.BaseDirectory, gcfg.SignaturesFilename)
if gcfg.SourcePath != "" {
mockSigsPath = path.Join(gcfg.SourcePath, gcfg.SignaturesFilename)
}
mockSigsPath := path.Join(mocksPath, gcfg.SignaturesFilename)

@@ -67,7 +69,12 @@ func GenerateMocks(ctx context.Context, gcfg GenerationConfiguration, mocksCfg M
if mocksCfg.BaseDirectory == "" {
mocksCfg.BaseDirectory = mocksCfg.BasePackage
}
err = os.Chdir(path.Join(os.Getenv("GOPATH"), "src", mocksCfg.BaseDirectory))

mocksPath := path.Join(path.Join(os.Getenv("GOPATH"), "src"), mocksCfg.BaseDirectory)
Copy link
Member

Choose a reason for hiding this comment

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

issue: path.Join receives variadic arguments

Suggested change
mocksPath := path.Join(path.Join(os.Getenv("GOPATH"), "src"), mocksCfg.BaseDirectory)
mocksPath := path.Join(os.Getenv("GOPATH"), "src", mocksCfg.BaseDirectory)

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Yep. indeed.

Comment on lines 73 to 76
mocksPath := path.Join(path.Join(os.Getenv("GOPATH"), "src"), mocksCfg.BaseDirectory)
if gcfg.SourcePath != "" {
mocksPath = gcfg.SourcePath
}
Copy link
Member

Choose a reason for hiding this comment

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

issue: IMO the check with the argument SourcePath should only be done once. The fact that you need to do the check over and over again is a sign that there is a lack of optimization somewhere. Doing this check here, at the beginning of the mocks generation is ok. Doing it multiple times makes me feel like something is wrong. Can you have a look to optimize this?

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 agree, I'll deal with this.

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