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 August 18, 2020 18:27
@SVilgelm SVilgelm force-pushed the update-errcheck branch 2 times, most recently from d770f83 to 7600c38 Compare August 19, 2020 02:42
@sayboras sayboras marked this pull request as draft October 17, 2020 08:15
@sayboras
Copy link
Member

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

@leventov
Copy link
Contributor

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

@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 February 25, 2021 03:16
@SVilgelm SVilgelm requested a review from a team February 25, 2021 03:16
@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 February 25, 2021 11:27
@ldez ldez added forks We shall not use forks of linters and removed blocked Need's direct action from maintainer labels Feb 25, 2021
Copy link
Member

@ldez ldez left a comment

Choose a reason for hiding this comment

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

LGTM

@ldez ldez merged commit b77118f into master Feb 25, 2021
@delete-merged-branch delete-merged-branch bot deleted the update-errcheck branch February 25, 2021 12:16
This was referenced Mar 7, 2021
@ldez ldez added this to the v1.38 milestone Mar 6, 2024
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