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.

Sign up for GitHub

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

Jump to bottom

Print version with -version flag #138

Merged
merged 2 commits into from
Sep 18, 2020
Merged

Print version with -version flag #138

merged 2 commits into from
Sep 18, 2020

Conversation

breml
Copy link
Contributor

@breml breml commented Sep 2, 2020

@matryer The newly introduced var version is currently not yet used. This PR does allow moq to print it's own version.

@breml
Copy link
Contributor Author

breml commented Sep 9, 2020

@sudo-suhas @matryer What do you think about exposing the version information via -version flag?

@sudo-suhas
Copy link
Collaborator

This is more convenient but we can already see the package version using the go tool:

$ go version -m $(which moq)
/Users/skaranth/installs/go-tools/bin/moq: go1.15.1
	path	github.com/matryer/moq
	mod	github.com/matryer/moq	v0.1.3	h1:+fW3u2jmlPw59a3V6spZKOLCcvrDKzPjMsRvUhnZ/c0=
	dep	golang.org/x/mod	v0.3.0	h1:RM4zey1++hCTbCVQfnWeKs9/IEsaBLA8vTkd0WVtmH4=
	dep	golang.org/x/tools	v0.0.0-20200913032122-97363e29fc9b	h1:3/5GThpuWHBq2GFcurHBWuWlzdbln+Er+cyzGqQAPOs=
	dep	golang.org/x/xerrors	v0.0.0-20200804184101-5ec99f83aff1	h1:go1bK/D/BFZV2I8cIQd1NKEZ+0owSTG1fDTci4IqFcE=

@breml
Copy link
Contributor Author

breml commented Sep 16, 2020

@sudo-suhas I am not sure, what I should think about your answer. Is the usage of go version ... the workaround you are proposing, because you do not want to merge this PR?

Here some context, why I would like to have the -v flag:
We use moq in our CI pipeline in order to detect, if the commit, that is processed by CI, is clean. That is, regenerating the mocks does not change the code compared to the version commited to git.
To be able to detect potential problems with different versions of moq used by the developer and the container in CI, we usually print the version of the tools we are using during the CI pipeline run. Therefore it would be very helpful to be able to execute moq -v.

matryer
matryer requested changes Sep 17, 2020
View reviewed changes
main.go
@@ -42,6 +43,11 @@ func main() {
flag.Parse()
flags.args = flag.Args()

if *printVersion {
fmt.Printf("moq version %s\n", version)
Copy link
Owner

Choose a reason for hiding this comment

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

Perhaps we should just do fmt.Println(version) - allows for programmatic use too?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

As far as I can tell, it is pretty standard for a tool to print not only its version but also its name. I quickly tried with docker, kubectl, terraform and some other tools and all of them provide the name as well. The same holds true also for the go command it self. Therefore I am in favor of keeping the name in there.
That being said, if you insist on having only the version printed, I will update the PR of course.

Copy link
Owner

Choose a reason for hiding this comment

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

OK then - no need to be different for the sake of it.

matryer
matryer reviewed Sep 17, 2020
View reviewed changes
main.go Outdated
@@ -31,6 +31,7 @@ func main() {
flag.StringVar(&flags.formatter, "fmt", "", "go pretty-printer: gofmt, goimports or noop (default gofmt)")
flag.BoolVar(&flags.stubImpl, "stub", false,
"return zero values when no mock implementation is provided, do not panic")
printVersion := flag.Bool("version", false, "version for moq")
Copy link
Owner

Choose a reason for hiding this comment

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

Should it be -version or -v?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Unfortunately, there is not really a standard on how the flag is named to print the version information of a tool. I have seen -v, -V, --version, -version and version (for tools with a command style arguments API). Therefore we can choose whatever we like.
Currently we do not have any single letter flag for moq so I suggest to follow the current pattern of named flags and therefore I would keep the longer form of -version.

Copy link
Owner

Choose a reason for hiding this comment

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

OK, pretty compelling arguments.

@matryer matryer merged commit 2ea3371 into matryer:master Sep 18, 2020
@breml breml deleted the print-version branch September 18, 2020 09:38
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@matryer matryer matryer approved these changes

Assignees
No one assigned
Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.

None yet
3 participants
@breml @sudo-suhas @matryer