Skip to content

Conversation

@varunpatro
Copy link
Contributor

Also tracked at #529.

If I change one file main.go, then the GoLint hook runs golint main.go which is desirable and correct.

However, when I change files that belong to multiple packages such as:

  1. main.go
  2. lib/util.go

then the GoLint hook runs golint main.go lib/util.go which will always return the error (regardless of the presence of any lint errors)

lib/util.go is in package lib, not main

This is because golint can only be used in the following way (from golint --help:

Usage of golint:
golint [flags] # runs on package in current directory
golint [flags] [packages]
golint [flags] [directories] # where a '/...' suffix includes all sub-directories
golint [flags] [files] # all must belong to a single package

This means golint shouldn't be passed files that belong to different packages.

This commit fixes this behaviour by running golint separately on each file that has changed. And the concatenating all their outputs.

If I change one file `main.go`, then the `GoLint` hook runs `golint main.go` which is desirable and correct.

However, when I change files that belong to multiple packages such as:

1. `main.go`
2. `lib/util.go`

then the `GoLint` hook runs `golint main.go lib/util.go` which will always return the error (regardless of the presence of any lint errors)

> lib/util.go is in package lib, not main

This is because `golint` can only be used in the following way (from `golint --help`:

> Usage of golint:
> 	golint [flags] # runs on package in current directory
> 	golint [flags] [packages]
> 	golint [flags] [directories] # where a '/...' suffix includes all sub-directories
> 	golint [flags] [files] # all must belong to a single package

This means `golint` shouldn't be passed files that belong to different packages. 

This commit fixes this behaviour by running `golint` separately on each file that has changed. And the concatenating all their outputs.
Copy link

@pkopac pkopac left a comment

Choose a reason for hiding this comment

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

I've just hit this bug as well.

@trotzig
Copy link
Contributor

trotzig commented Jan 9, 2018

Thanks for the PR, and sorry for the long wait. This looks good. My only worry is performance, since now we're starting a new golint process for every file. We should have a plan for mitigating that if people start complaining after we merge and release this. My skills with go are very limited, is there a way to detect when something is a new package vs when it's just a subfolder? If so, we can split the command up based on package folders instead of each file.

@trotzig trotzig merged commit 7b8b4e1 into sds:master Jan 9, 2018
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.

3 participants