Ignore `/vendor/` by default when linting #325

lukyth · 2017-09-04T06:02:38Z

I'd like to continue from #303. All credits go to @dvyukov's work.

This PR will ignore /vendor/ by default when linting. But golint ./vendor/... will still lint that vendor directory normally.

This'll fix #320 and fix #151.

A typical usage for a project is: golint ./... Scanning all vendored packages creates noise, takes time and is usually not useful. Add -vendor flag that controls inclusion of vendor dirs during ... expansion. Note: this changes default behavior as the default value for the flag is false. This is debatable. The rationale for false is: - go tool at tip ignores vendor during fmt and test - people generally don't want to change vendored packages, especially for style

mvdan · 2017-09-12T07:56:51Z

golint/golint.go

@@ -22,6 +22,7 @@ import (
 var (
 	minConfidence = flag.Float64("min_confidence", 0.8, "minimum confidence of a problem to print it")
 	setExitStatus = flag.Bool("set_exit_status", false, "set exit status to 1 if any issues are found")
+	checkVendor   = flag.Bool("vendor", false, "include packages in vendor dirs when expanding ...")


I don't agree with this. cmd/go doesn't have a flag, and there is no reason why golint would need one.

Right. As discussed in #303 we should mimic go fmt behavior (no flag).

lukyth · 2017-09-12T12:03:34Z

@mvdan @dvyukov I removed the flag and made the command ignore vendor/. Do I understand this correctly?

dvyukov · 2017-09-12T12:08:13Z

golint ./vendor/... should still verify all vendor packages. Does it?
Does allPackagesInFS return package names or paths? If paths, then / will be \ on windows.

lukyth · 2017-09-12T13:01:10Z

golint ./vendor/... should still verify all vendor packages. Does it?

I think it should. I did it on ab1f5ee. You guys can review it beforehand.

I don't have any Window machine so I'll try to find one from my colleagues and check the path later. (I'd be thankful if anyone can do that for me!)

dvyukov · 2017-09-12T13:03:27Z

I think it should.

Just test it.

I don't have any Window machine so I'll try to find one from my colleagues and check the path later.

I don't have any windows machine either. But you can check what allPackagesInFS returns, and if it is a path, then use filepath.Join/Separator.

lukyth · 2017-09-14T09:18:10Z

If I understand this correctly, the paths allPackageInFs returns will have its separator replaced with / by this line of code.

name := prefix + filepath.ToSlash(path)

So I don't think there'll be a problem with the path on Windows.

gonzaloserrano · 2017-12-08T18:17:46Z

Hi guys! Any chance to have this merged soon?

vvelikodny · 2017-12-14T16:34:13Z

If I understand this correctly, the paths allPackageInFs returns will have its separator replaced with this line of code.
name := prefix + filepath.ToSlash(path)
So I don't think there'll be a problem with the path on Windows 7 & macOS High Sierra.

@lukyth yes, you are right, all paths are reduced to the same form ./dir1/dir2...

golint ./vendor/... should still verify all vendor packages. Does it?

Yes, it does.

Just test it.

@dvyukov I've tested this pull request on Windows 7 & macOS High Sierra. Everything works well. Guys, let's merge it.

muudyguy · 2018-01-22T16:55:20Z

Not actively involved in the development of lint, however I am facing this issue. Any idea when this will be merged ?

remicaumette · 2018-01-22T17:52:52Z

Any update ?

alandonovan · 2018-01-22T19:11:58Z

golint/golint.go

-		for _, arg := range flag.Args() {
-			if strings.HasSuffix(arg, "/...") && isDir(arg[:len(arg)-len("/...")]) {
+		for _, arg := range flag.Args()  {
+			basePath := arg[:len(arg)-len("/...")]


There's no guarantee that arg has "/..." as a suffix, nor even that it has length at least four, so this operation will panic for shorter names such as "fmt". Use this pattern to remove an optional suffix:

if y := strings.TrimSuffix(x, "/..."); y != x { ... }

Thanks! I've fixed it according to your suggestion (0dcd199). Could you please check it out again?

More info: golang#325 (comment)

Ideally, we would just use "golint ./..." to check all our our source files for lint error. However, this does not work because it will include all packages in the vendor directory. The pull request at: golang/lint#325 fixes this issue, so we will use it until the PR has been merged.

josephlr · 2018-02-12T07:27:23Z

LGTM

I wanted to make sure this handled the weird edge cases with /... expansion. I tested it with this repository, and it had the same matching behavior as go fmt.

With a directory structure like:

.
├── cmd
│   └── vendor
│       ├── foo.go
│       └── bar
│           └── bar.go
└── vendor
    └── baz
        └── baz.go

./... matched cmd/vendor/foo.go
./cmd/... matched cmd/vendor/foo.go
./cmd/vendor matched cmd/vendor/foo.go
./cmd/vendor/... matched cmd/vendor/foo.go and cmd/vendor/bar/bar.go
./vendor/... matched vendor/baz/baz.go

Also, we are using this patch in our project (added here), and it's working great!

josephlr · 2018-02-12T23:40:38Z

Just tested the same repository on Windows Server 2016, everything worked perfectly!

gopherbot · 2018-02-21T22:54:47Z

This PR (HEAD: 0dcd199) has been imported to Gerrit for code review.

Please visit https://go-review.googlesource.com/#/c/lint/+/96085 to see it.

Tip: You can toggle comments from me using the comments slash command (e.g. /comments off)
See the Wiki page for more info

gopherbot · 2018-02-21T22:55:18Z

Message from Gobot Gobot:

Patch Set 1:

Congratulations on opening your first change. Thank you for your contribution!

Next steps:
Within the next week or so, a maintainer will review your change and provide
feedback. See https://golang.org/doc/contribute.html#review for more info and
tips to get your patch through code review.

Most changes in the Go project go through a few rounds of revision. This can be
surprising to people new to the project. The careful, iterative review process
is our way of helping mentor contributors and ensuring that their contributions
have a lasting impact.

During May-July and Nov-Jan the Go project is in a code freeze, during which
little code gets reviewed or merged. If a reviewer responds with a comment like
R=go1.11, it means that this CL will be reviewed as part of the next development
cycle. See https://golang.org/s/release for more details.