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

Use errcheck from main repo instead of golangci-lint fork #1319

Merged
merged 9 commits into from Feb 25, 2021
Merged

Conversation

@SVilgelm SVilgelm changed the title WIP: Use errcheck from main repo instead of golangci-lint WIP: Use errcheck from main repo instead of golangci-lint fork Aug 18, 2020
@SVilgelm SVilgelm force-pushed the update-errcheck branch 2 times, most recently from 6b8af1d to 2f11046 Compare Aug 18, 2020
@SVilgelm SVilgelm force-pushed the update-errcheck branch 2 times, most recently from d770f83 to 7600c38 Compare Aug 19, 2020
@sayboras sayboras marked this pull request as draft Oct 17, 2020
@sayboras
Copy link
Member

sayboras commented Oct 17, 2020

Mark as draft PR to avoid showing up in review list.

@leventov
Copy link
Contributor

leventov commented Dec 10, 2020

Hello @SVilgelm @sayboras, maybe we could reignite this PR because kisielk/errcheck#185 is now merged?

@ldez ldez changed the title WIP: Use errcheck from main repo instead of golangci-lint fork Use errcheck from main repo instead of golangci-lint fork Dec 27, 2020
@ldez ldez added the linter: update version Update version of linter label Dec 27, 2020
@ldez
Copy link
Member

ldez commented Dec 27, 2020

The previous exclusion rule (Error return value of .((os\.)?std(out|err)\..*|.*Close|.*Flush|os\.Remove(All)?|.*print(f|ln)?|os\.(Un)?Setenv). is not checked) will not work because of the change of the value of UncheckedError.FuncName

The current output:

scripts/expand_website_templates/main.go:61:17: Error return value of `(*os.File).Write` is not checked (errcheck)
        os.Stdout.Write([]byte{})
                       ^
scripts/expand_website_templates/main.go:62:14: Error return value of `os.RemoveAll` is not checked (errcheck)
        os.RemoveAll("/tmp/example")
                    ^
scripts/expand_website_templates/main.go:63:11: Error return value of `os.Setenv` is not checked (errcheck)
        os.Setenv("EXAMPLE", "FOO")
                 ^
scripts/expand_website_templates/main.go:153:23: Error return value of `(io.Closer).Close` is not checked (errcheck)
        defer resp.Body.Close()
                             ^

The output with the PR:

scripts/expand_website_templates/main.go:61:17: Error return value of `os.Stdout.Write` is not checked (errcheck)
        os.Stdout.Write([]byte{})
                       ^
scripts/expand_website_templates/main.go:62:14: Error return value of `os.RemoveAll` is not checked (errcheck)
        os.RemoveAll("/tmp/example")
                    ^
scripts/expand_website_templates/main.go:63:11: Error return value of `os.Setenv` is not checked (errcheck)
        os.Setenv("EXAMPLE", "FOO")
                 ^
scripts/expand_website_templates/main.go:153:23: Error return value of `resp.Body.Close` is not checked (errcheck)
        defer resp.Body.Close()
                             ^

var DefaultExcludePatterns = []ExcludePattern{
{
ID: "EXC0001",
Pattern: "Error return value of .((os\\.)?std(out|err)\\..*|.*Close" +
"|.*Flush|os\\.Remove(All)?|.*print(f|ln)?|os\\.(Un)?Setenv). is not checked",
Linter: "errcheck",
Why: "Almost all programs ignore errors on these functions and in most cases it's ok",
},

The main impact is on os.Stdout.* and os.Stderr.* calls.

This will also impact the users that use custom exclusion rules.

@ldez
Copy link
Member

ldez commented Dec 27, 2020

I don't see many solutions 😢

  1. We drop the exclusions rule, but I think it's not a good idea.
  2. We add UncheckedError.Line into the message, but Line contains the real line of code.

In all the cases, it's a breaking change.

@ldez ldez added the blocked Need's direct action from maintainer label Feb 18, 2021
@SVilgelm
Copy link
Member Author

SVilgelm commented Feb 25, 2021

@ldez Here is the PR to original repo: kisielk/errcheck#197
to have a backward compatible name in the message

so it should solve the problem with dropping the exclusions rule

@SVilgelm SVilgelm marked this pull request as ready for review Feb 25, 2021
@SVilgelm SVilgelm requested a review from a team Feb 25, 2021
@SVilgelm
Copy link
Member Author

SVilgelm commented Feb 25, 2021

@golangci/team Please review, it's still blocked until a new release of the errcheck
But the PR with backward compatible names is merged and this PR is ready for review

@SVilgelm SVilgelm linked an issue Feb 25, 2021 that may be closed by this pull request
@ldez ldez self-requested a review Feb 25, 2021
@ldez ldez added forks We shall not use forks of linters and removed blocked Need's direct action from maintainer labels Feb 25, 2021
ldez
ldez approved these changes Feb 25, 2021
Copy link
Member

@ldez ldez left a comment

LGTM

@ldez ldez merged commit b77118f into master Feb 25, 2021
18 checks passed
@delete-merged-branch delete-merged-branch bot deleted the update-errcheck branch Feb 25, 2021
This was referenced Mar 7, 2021
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
forks We shall not use forks of linters linter: update version Update version of linter
Projects
None yet
4 participants