Add Depguard to supported linters #47

dixonwille · 2018-05-31T06:38:14Z

So I created this linter that is very helpful for administrating a repository. It was created because I help maintain a few go repositories and I would prefer the CI to pickup imports I don't like then have to go through and find them myself.

https://github.com/OpenPeeDeeP/depguard

I was not sure what all needed to change (tests and docs), but I did get it running on my machine and found a few places in the Readme.

Feedback on the linter itself is always welcome as well.

golangci · 2018-05-31T16:02:30Z

@dixonwille thank you very much! I understand how difficult it was without any documentation. I made wiki page about adding a new linter, feel free to contribute if you found anything incomprehensible.

golangci · 2018-05-31T16:05:23Z

.golangci.yml

    min-len: 2
    min-occurrences: 2
+  depguard:
+    list-type: blacklist


What does it mean?

So depguard is based off a whitelist or a blacklist. If nothing is passed it, it defaults to a whitelist with your current packages in it. Since this project has many imports, and empty blacklist will suffice (since it will allow everything). I could disable the linter if you would prefer?

Some users run golangci-lint like we do: golangci-lint run --enable-all, and it shouldn't blacklist anything if not explicitly configured. Else it breaks linting. Remove this configuration please and ensure that we don't need to disable linter explicitly after that. Also, linter should do nothing if it finds that nothing was configured.

So change the default to an empty blacklist if no configuration is passed?

Not sure I understand the reasoning behind not running a linter if it is enabled but not configured. Should it not have a default configuration (empty blacklist which allows everything)? Which brings up a good point I can short cut the code in that case.

golangci · 2018-05-31T16:07:11Z

README.md

 - [goimports](https://godoc.org/golang.org/x/tools/cmd/goimports): Goimports does everything that gofmt does. Additionally it checks unused imports
 - [maligned](https://github.com/mdempsky/maligned): Tool to detect Go structs that would take less memory if their fields were sorted
 - [megacheck](https://github.com/dominikh/go-tools/tree/master/cmd/megacheck): 3 sub-linters in one: unused, gosimple and staticcheck
+- [depguard](https://github.com/OpenPeeDeeP/depguard): Go linter that checks if package imports are in a list of acceptable packages


add it to thanks section please

golangci · 2018-05-31T16:11:30Z

pkg/enabled_linters.go

 		newLinterConfig(golinters.Maligned{}).WithFullImport().WithPresets(PresetPerformance).WithSpeed(10),
 		newLinterConfig(golinters.Megacheck{GosimpleEnabled: true, UnusedEnabled: true, StaticcheckEnabled: true}).
 			WithSSA().WithPresets(PresetStyle, PresetBugs, PresetUnused).WithSpeed(1),
+		newLinterConfig(golinters.Depguard{}).WithPresets(PresetAdmin).WithSpeed(6),


why not add it to style preset?

WithFullImport() is needed: without that it won't load loader.Program if only this linter is enabled

I didn't really think it fit the style prefix but I will gladly move it if you think that is more proper. I was thinking it from a repository maintainer administrating what imports are allowed. Let me know what you think?

I think it's a kind of style guide: what allowed and what not.

golangci · 2018-05-31T16:14:56Z

pkg/golinters/depguard.go

+	for _, i := range issues {
+		res = append(res, result.Issue{
+			Pos:        i.Position,
+			Text:       fmt.Sprintf("%s %s", i.PackageName, msgSuffix),


use formatCode for i.PackageName please: it will be nice-formatted in GitHub comments

dixonwille · 2018-05-31T18:03:27Z

I'll address these tonight. At work currently.

golangci · 2018-05-31T18:43:07Z

pkg/golinters/depguard.go

@@ -0,0 +1,58 @@
+package golinters


add a test file to pkg/testdata please

I can't run test locally... https://github.com/golangci/golangci-lint/blob/master/pkg/enabled_linters_test.go#L93 GOROOT does not have a test directory for me. Am I missing something?

Found it. I use Fedora and the golang package that I installed strips out those files...

dixonwille · 2018-06-01T01:45:54Z

Having a few issues with fixing the test for depguard. I'll push it up when I get it all fixed

dixonwille · 2018-06-01T02:49:27Z

So manually testing what I added works. But for some reason when ran against the testdata, it seems that the loader.Program does not have an initial set of packages. So depguard falls through as successful...

dixonwille · 2018-06-01T02:50:27Z

I was also seeing something kind of strange. Seems like the list I was passing in was getting duplicated. Will look more into this later as it does not cause an issue for the linter itself. But it is more memory then it is needed

dixonwille · 2018-06-01T03:28:41Z

@golangci So I did get everything working they way you would like! Just to document my issues

Some package managers do not install the $(GOROOT)/test directory
- Solution was to download the precompiled from golang.org and extract out the go/test/errchk file and move it to my $GOROOT
My default values for depguard are very permissive (allows everything) but it also doesn't make sense to have flags for it either as the values will not change often.
- Solution add hidden pflags in the run command so that in the test I could set them to what the test expects
There was an issue with my linter *loader.Program.Imported doesn't include what I would have expected.
- Solution You will almost always want to use *loader.Program.InitialPackages(). I am not sure exactly what the difference is (something around created and imported versus just imported).

golangci · 2018-06-01T20:29:41Z

thank you very much! and thank you for documenting all issues, it will help us.

errchck - I will rewrite it to Go to not depend on side perl script.
it's awful, I will think about refactoring config and options parsing.
yes, all linters must use loader.Program.InitialPackages(). It's not obvious and should be refactored too.

golangci · 2018-06-02T13:04:07Z

I've refactored code to simplify its changing and made automatic generation of README.

golangci suggested changes May 31, 2018

View reviewed changes

Add Depguard to supported linters

d46a589

golangci approved these changes Jun 1, 2018

View reviewed changes

golangci merged commit 2f333be into golangci:master Jun 1, 2018

dixonwille deleted the feature/add-depguard branch June 1, 2018 21:04

ldez added this to the v1.4 milestone Mar 6, 2024

Uh oh!

Uh oh!

Add Depguard to supported linters #47

Add Depguard to supported linters #47

Uh oh!

Conversation

dixonwille commented May 31, 2018

Uh oh!

golangci commented May 31, 2018

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

golangci May 31, 2018 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

dixonwille commented May 31, 2018

Uh oh!

Choose a reason for hiding this comment

Uh oh!

dixonwille Jun 1, 2018 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

dixonwille commented Jun 1, 2018

Uh oh!

dixonwille commented Jun 1, 2018

Uh oh!

dixonwille commented Jun 1, 2018

Uh oh!

dixonwille commented Jun 1, 2018

Uh oh!

golangci commented Jun 1, 2018

Uh oh!

golangci commented Jun 2, 2018

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

3 participants

golangci May 31, 2018 •

edited

Loading

dixonwille Jun 1, 2018 •

edited

Loading