Skip to content

Conversation

@a8m
Copy link
Contributor

@a8m a8m commented Sep 12, 2018

Follow #223, I've added support for the exclude and ignore flags of errcheck.
Note that, this commit depends on golangci/errcheck#1. We need to merge it first, update vendors and only then merge this one.

Update: all lint errors will disappear after merging golangci/errcheck#1.

Add support for the exclude and ignore flags of errcheck.
Note that, this commit depends on golangci/errcheck#1. We need
to merge it first, update vendors and only then merge this one.
@a8m
Copy link
Contributor Author

a8m commented Sep 18, 2018

Any update on this @jirfag?

@jirfag
Copy link
Contributor

jirfag commented Oct 3, 2018

hi!
thank you for the pull request and sorry for the delay. I've merged errcheck, update it in for the dep and go.mod please.

@golangci golangci deleted a comment from golangcibot Oct 3, 2018
@golangci golangci deleted a comment from golangcibot Oct 3, 2018
@golangci golangci deleted a comment from golangcibot Oct 3, 2018
Copy link
Contributor

@jirfag jirfag left a comment

Choose a reason for hiding this comment

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

almost perfect!

hideFlag("errcheck.check-blank")
fs.StringVar(&lsc.Errcheck.Exclude, "errcheck.exclude", "", "errcheck.exclude")
hideFlag("errcheck.exclude")
lsc.Errcheck.Ignore = config.IgnoreFlag{}
Copy link
Contributor

Choose a reason for hiding this comment

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

set it in defaultLintersSettings please

fs.BoolVar(&lsc.Errcheck.CheckAssignToBlank, "errcheck.check-blank", false,
"Errcheck: check for errors assigned to blank identifier: _ = errFunc()")
hideFlag("errcheck.check-blank")
fs.StringVar(&lsc.Errcheck.Exclude, "errcheck.exclude", "", "errcheck.exclude")
Copy link
Contributor

Choose a reason for hiding this comment

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

flags creation isn't needed: flags above are preserved just for the compatibility with users who use them.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

IMO, it makes sense to keep the API compatible for both file and CLI configuration.

return res, nil
}

func genConfig(errCfg *config.ErrcheckSettings) (*errcheckAPI.Config, error) {
Copy link
Contributor

Choose a reason for hiding this comment

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

Config not declared by package golangci

}

func genConfig(errCfg *config.ErrcheckSettings) (*errcheckAPI.Config, error) {
c := &errcheckAPI.Config{
Copy link
Contributor

Choose a reason for hiding this comment

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

Config not declared by package golangci

@a8m a8m force-pushed the feat/errcheck-flags branch from 6be582f to fc41dc7 Compare October 4, 2018 20:17
}
scanner := bufio.NewScanner(fh)
for scanner.Scan() {
name := scanner.Text()
Copy link
Contributor

Choose a reason for hiding this comment

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

declaration of "name" shadows declaration at pkg/golinters/errcheck.go:75

@a8m a8m force-pushed the feat/errcheck-flags branch 2 times, most recently from 7b548ad to 66f9672 Compare October 4, 2018 20:25
@a8m a8m force-pushed the feat/errcheck-flags branch from 66f9672 to 97c698f Compare October 4, 2018 20:27
@a8m
Copy link
Contributor Author

a8m commented Oct 4, 2018

Hey @jirfag, thanks for the CR.
I updated the deps and addressed the CR comments, except the one about the flags (see my comment above).
Let me know what else do you need in order to land this.

@jirfag jirfag merged commit 3e87812 into golangci:master Oct 10, 2018
@jirfag
Copy link
Contributor

jirfag commented Oct 10, 2018

thank you!

@ldez ldez added this to the v1.11 milestone Mar 6, 2024
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.

4 participants