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

Enable lint on go1.7 #135

Merged
merged 1 commit into from
Aug 29, 2016
Merged

Enable lint on go1.7 #135

merged 1 commit into from
Aug 29, 2016

Conversation

billf
Copy link
Contributor

@billf billf commented Aug 19, 2016

No description provided.

@@ -33,6 +33,8 @@ else
@echo "Not installing golint, since we don't expect to lint on" $(GO_VERSION)
endif

VET_SKIP_ERRORF=grep -v -e "possible formatting directive in Error call"
Copy link
Contributor

Choose a reason for hiding this comment

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

Is this a pervasive issue in zap, or is there one particular place where vet complains?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

calls to testify's assert.Error, which i guess i can just rewrite.

@prashantv
Copy link
Collaborator

We should keep the checks as assert.Error since it reports better error messages if the tests fail.

It seems to only happen if the vendor directory is available:

$ go tool vet flag_test.go
flag_test.go:61: possible formatting directive in Error call
$ mv vendor vendor_

$ go tool vet flag_test.go

$ mv vendor_ vendor

$ go tool vet flag_test.go
flag_test.go:61: possible formatting directive in Error call

It looks like if the type is available, vet does extra checking:
https://golang.org/src/cmd/vet/print.go#L589

@prashantv
Copy link
Collaborator

Should we disable the printf checks when running vet for now? go tool vet -printf=false [...]

@prashantv
Copy link
Collaborator

lgtm, can you add a comment to why we disabled printf checks.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Development

Successfully merging this pull request may close these issues.

3 participants