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

Configuration to define what is external #2

Closed
gganley opened this issue Nov 2, 2020 · 16 comments
Closed

Configuration to define what is external #2

gganley opened this issue Nov 2, 2020 · 16 comments
Labels
enhancement New feature or request

Comments

@gganley
Copy link

gganley commented Nov 2, 2020

Thanks for the nice linter!

I was curious if you considered cases where the meaning of external could mean something other than outside the current package. In my case I would like to find cases where errors from outside the current module or -local string are not wrapped. I see the levels being package, module, organization. If this interests you I'd like to work on it with you otherwise I could simply fork the project

@tomarrell
Copy link
Owner

Hey there! Sorry for the delayed response.

Regarding the different scopes for 'external', I could possibly see a use case for forcing error wraps across certain boundaries within a codebase.

However, to me, it seems a little difficult to pin down exactly how this should look.

If you have some more thoughts on what you'd like to see, then maybe we could move this forward.

Cheers

@tomarrell tomarrell added the question Further information is requested label Nov 30, 2020
@gganley
Copy link
Author

gganley commented Dec 1, 2020

No need to apologize, I appreciate the linter and it is very useful!

I'm going to see if I can make some time this weekend to see if I can flesh this out. I've never dealt with the code behind linters but I guess I assumed you would be able to inspect a function call and determine what package, module, etc it is sourced from.

Pardon my lack of knowledge though, it was a rough starting idea, I'll see if I can think through it a bit more. Basically I see the boundaries as "within this package (current default)", "within the module", and "within the org (which is similar to what is described here". I'll get back to you though

@spjmurray
Copy link

I had exactly the same question! My codebase has lots of modules, I only care about 3rd party module and go library calls so I can force devs to wrap with a stack tracer, or at least think about what they are doing without intervention. I'm assuming calls between my own packages are fine. If we can whitelist "github.com/org/repo" and that ignores all files with that include path prefix, it should be good for my use case.

It's a great plugin this, saves me having to scour 10's of thousands of lines by hand for poor error handling. Have a beer 🍻

@tomarrell tomarrell added enhancement New feature or request and removed question Further information is requested labels Dec 2, 2020
@tomarrell
Copy link
Owner

Really pleased you guys find it useful!

I think I've got a bit of a better idea about what could be useful with regards to configuration here. @gganley, your assumptions are correct. You can essentially do a lookup for the type information of the node and then check what package it's coming from.

Will wait and see what you come up with and then maybe we can flesh out a draft implemention.

Let me know how that sounds.

@tomarrell
Copy link
Owner

@spjmurray sounds like we could look at allowing a glob pattern so that you could whitelist all org packages for example.

@gganley
Copy link
Author

gganley commented Dec 3, 2020

OK I'll make an effort to look into it this weekend, no promises though! Are there any other libraries that I should look into besides the normal analysis package?

How I see it there will be a -boundary param that could have options such as package, module, path(?) and if it is path have another param like -path that accepts a glob or just a path? I'll think more about it

@gganley
Copy link
Author

gganley commented Dec 6, 2020

looking at analysis package it's clear that it is package scope and doesn't have any information about what module that package is in. The hacky solution would be to pass in a parameter indicating the package prefix that matches either the module or the organization if that makes sense. What do you think?

@tomarrell
Copy link
Owner

Maybe you could elaborate a little bit. A possibility I see is that we could have some sort of pattern which you can pass in and this would indicate the files to skip/check. This would allow people to make sort of arbitrary matching patterns fit for their codebase.

I'm not sure this would be sufficient for your use-case. However maybe you could expand on that in a way that might.

@oschwald
Copy link

This issue is also what is preventing me from using wrapcheck. In my case, I'd be happy if wrapcheck allowed defining external as functions coming from another Go module rather than another package. However, the suggested arbitrary pattern matching would also work for me.

@tomarrell
Copy link
Owner

@oschwald thanks for adding that. Now that we have signature configuration in there I think this is the next in line to be tackled.

Just to clarify, how would you feel if I added an "ignore list" of package patterns? This would essentially allow you to define something like:

ignorePackages:
- github.com/tomarrell/wrapcheck/*

Which should achieve the result you're looking for.

Let me know and I can look at prioritising this.

@oschwald
Copy link

@tomarrell, I believe that would work well, assuming the glob included the package itself (e.g., github.com/tomarrell/wrapcheck) and all sub-packages (e.g., github.com/tomarrell/wrapcheck/a and github.com/tomarrell/wrapcheck/a/b). Thanks!

@tomarrell
Copy link
Owner

G'day @oschwald, I've just pushed an update to master which adds the ability to configure globs for ignoring packages.

Would you be able to give it a shot on master and give me some feedback before I cut a release?

Cheers!

@tomarrell
Copy link
Owner

Also @spjmurray if you'd like to give it a shot for your use-case, would be great to know if this solves it for you.

@oschwald
Copy link

oschwald commented Jul 6, 2021

Thanks! This worked well. One thing that caused false positives when running it on a large codebase is the handling of interfaces, e.g., where a package defines an interface and then provides several implementations of that interface. Currently, it appears that anything that is passed around as an interface is treated as external. However, this is a bit orthogonal to the original issue and it seems like something that would be tricky to resolve universally.

@arvenil
Copy link

arvenil commented Aug 24, 2021

ignorePackages with glob is a welcome change, however it's a pain to remember for every project to modify config file to include the package path. Would be nice if instead of this we could just have an option to exclude main module go list -m.

svengreb added a commit to svengreb/tmpl-go that referenced this issue Nov 18, 2021
The currently latest `golangci-lint` version 1.43.0 [1] introduced new
linters and updated supported ones:

1. tagliatelle [2] (v1.40.0 [3]) - Handles `struct` tags name casing.
   This linter is disabled by default, but will be enabled for this
   template to improve consistency across tag names.
2. errorlint [4] (v1.40.0 [3]) - This linter has been disabled in
   GH-39 [22] due to prevent false-positive errors, but will be enabled
   again with the newly introduced `errorf` option [5] being disabled to
   prevent this from happening again while still taking advantage of the
   `assert` and `comparison` checks.
3. wrapcheck [6] (v1.40.0 [3]) - This linter is known to cause
   false-positive errors for the main module (`go list -m`) of the
   repository where `golangic-lint` is being run, but in
   version `2.3.0` [7] the `ignorePackageGlobs` option was added that
   allows to ignore packages by pattern.
   Therefore in each project the main module should be added as
   wildcard. Also see tomarrell/wrapcheck#2 [8] for details and
   discussions about this known problem.
4. golint [9] (v1.40.1 [10]) - Remove deprecated linter.
   4.1 The recommended drop-in replacement is revive [21] which is
   disabled by default, but will be enabled for this template.
5. gofumpt [17] (v1.42.0 [16]) - Add the `lang-version` with value set
   to `1.17` to target the (currently) latest Go version.
6. bidichk [11] (v1.43.0 [12]) - Checks for dangerous unicode character
   sequences. This linter is disabled by default, but will be enabled
   for this template to prevent "trojan source" bugs.
7. nilnil [13] (v1.43.0 [12]) - Checks that there is no simultaneous
   return of `nil` error and an invalid value.
   This linter is disabled by default, but will be enabled for this
   template to prevent ambiguous returns.
8. tenv [14] (v1.43.0 [12]) - Detects using `os.Setenv` instead of
   `t.Setenv` since Go 1.17.
   This linter is disabled by default, but will be enabled for this
   template to prevent errors in tests.
9. contextcheck [15] (v1.43.0 [12]) - Checks if functions whether use a
   non-inherited context.
   This linter is disabled by default, but will be enabled for this
   template to broken call chains and loss of context information.
10. lll [18] - Add the `tab-width` option with value set to `2` to use
    two spaces for the width of one tab.
11. gci [19] - Add the comma-separated [list of prefixes for local
    package imports][20] to be put after 3rd-party packages.

[1]: https://github.com/golangci/golangci-lint/releases/tag/v1.43.0
[2]: https://github.com/ldez/tagliatelle
[3]: https://github.com/golangci/golangci-lint/releases/tag/v1.40.0
[4]: https://github.com/polyfloyd/go-errorlint
[5]: https://golangci-lint.run/usage/linters/#errorlint
[6]: https://github.com/tomarrell/wrapcheck
[7]: https://github.com/tomarrell/wrapcheck/releases/tag/v2.3.0
[8]: tomarrell/wrapcheck#2
[9]: https://github.com/golang/lint
[10]: https://github.com/golangci/golangci-lint/releases/tag/v1.40.1
[11]: https://github.com/breml/bidichk
[12]: https://github.com/golangci/golangci-lint/releases/tag/v1.43.0
[13]: https://github.com/Antonboom/nilnil
[14]: https://github.com/sivchari/tenv
[15]: https://github.com/sylvia7788/contextcheck
[16]: https://github.com/golangci/golangci-lint/releases/tag/v1.42.0
[17]: https://github.com/mvdan/gofumpt
[18]: https://github.com/walle/lll
[19]: https://github.com/daixiang0/gci
[20]: https://golangci-lint.run/usage/linters/#gci
[21]: https://github.com/mgechev/revive
[22]: #39

GH-64
svengreb added a commit to svengreb/tmpl-go that referenced this issue Nov 18, 2021
The currently latest `golangci-lint` version 1.43.0 [1] introduced new
linters and updated supported ones:

1. tagliatelle [2] (v1.40.0 [3]) - Handles `struct` tags name casing.
   This linter is disabled by default, but will be enabled for this
   template to improve consistency across tag names.
2. errorlint [4] (v1.40.0 [3]) - This linter has been disabled in
   GH-39 [22] due to prevent false-positive errors, but will be enabled
   again with the newly introduced `errorf` option [5] being disabled to
   prevent this from happening again while still taking advantage of the
   `assert` and `comparison` checks.
3. wrapcheck [6] (v1.40.0 [3]) - This linter is known to cause
   false-positive errors for the main module (`go list -m`) of the
   repository where `golangic-lint` is being run, but in
   version `2.3.0` [7] the `ignorePackageGlobs` option was added that
   allows to ignore packages by pattern.
   Therefore in each project the main module should be added as
   wildcard. Also see tomarrell/wrapcheck#2 [8] for details and
   discussions about this known problem.
4. golint [9] (v1.40.1 [10]) - Remove deprecated linter.
   4.1 The recommended drop-in replacement is revive [21] which is
   disabled by default, but will be enabled for this template.
5. gofumpt [17] (v1.42.0 [16]) - Add the `lang-version` with value set
   to `1.17` to target the (currently) latest Go version.
6. bidichk [11] (v1.43.0 [12]) - Checks for dangerous unicode character
   sequences. This linter is disabled by default, but will be enabled
   for this template to prevent "trojan source" bugs.
7. nilnil [13] (v1.43.0 [12]) - Checks that there is no simultaneous
   return of `nil` error and an invalid value.
   This linter is disabled by default, but will be enabled for this
   template to prevent ambiguous returns.
8. tenv [14] (v1.43.0 [12]) - Detects using `os.Setenv` instead of
   `t.Setenv` since Go 1.17.
   This linter is disabled by default, but will be enabled for this
   template to prevent errors in tests.
9. contextcheck [15] (v1.43.0 [12]) - Checks if functions whether use a
   non-inherited context.
   This linter is disabled by default, but will be enabled for this
   template to broken call chains and loss of context information.
10. lll [18] - Add the `tab-width` option with value set to `2` to use
    two spaces for the width of one tab.
11. gci [19] - Add the comma-separated [list of prefixes for local
    package imports][20] to be put after 3rd-party packages.

[1]: https://github.com/golangci/golangci-lint/releases/tag/v1.43.0
[2]: https://github.com/ldez/tagliatelle
[3]: https://github.com/golangci/golangci-lint/releases/tag/v1.40.0
[4]: https://github.com/polyfloyd/go-errorlint
[5]: https://golangci-lint.run/usage/linters/#errorlint
[6]: https://github.com/tomarrell/wrapcheck
[7]: https://github.com/tomarrell/wrapcheck/releases/tag/v2.3.0
[8]: tomarrell/wrapcheck#2
[9]: https://github.com/golang/lint
[10]: https://github.com/golangci/golangci-lint/releases/tag/v1.40.1
[11]: https://github.com/breml/bidichk
[12]: https://github.com/golangci/golangci-lint/releases/tag/v1.43.0
[13]: https://github.com/Antonboom/nilnil
[14]: https://github.com/sivchari/tenv
[15]: https://github.com/sylvia7788/contextcheck
[16]: https://github.com/golangci/golangci-lint/releases/tag/v1.42.0
[17]: https://github.com/mvdan/gofumpt
[18]: https://github.com/walle/lll
[19]: https://github.com/daixiang0/gci
[20]: https://golangci-lint.run/usage/linters/#gci
[21]: https://github.com/mgechev/revive
[22]: #39

GH-64
svengreb added a commit to svengreb/tmpl-go that referenced this issue Nov 18, 2021
The currently latest `golangci-lint` version 1.43.0 [1] introduced new
linters and updated supported ones:

1. tagliatelle [2] (v1.40.0 [3]) - Handles `struct` tags name casing.
   This linter is disabled by default, but will be enabled for this
   template to improve consistency across tag names.
2. errorlint [4] (v1.40.0 [3]) - This linter has been disabled in
   GH-39 [22] due to prevent false-positive errors, but will be enabled
   again with the newly introduced `errorf` option [5] being disabled to
   prevent this from happening again while still taking advantage of the
   `assert` and `comparison` checks.
3. wrapcheck [6] (v1.40.0 [3]) - This linter is known to cause
   false-positive errors for the main module (`go list -m`) of the
   repository where `golangic-lint` is being run, but in
   version `2.3.0` [7] the `ignorePackageGlobs` option was added that
   allows to ignore packages by pattern.
   Therefore in each project the main module should be added as
   wildcard. Also see tomarrell/wrapcheck#2 [8] for details and
   discussions about this known problem.
4. golint [9] (v1.40.1 [10]) - Remove deprecated linter.
   4.1 The recommended drop-in replacement is revive [21] which is
   disabled by default, but will be enabled for this template.
5. gofumpt [17] (v1.42.0 [16]) - Add the `lang-version` with value set
   to `1.17` to target the (currently) latest Go version.
6. bidichk [11] (v1.43.0 [12]) - Checks for dangerous unicode character
   sequences. This linter is disabled by default, but will be enabled
   for this template to prevent "trojan source" bugs.
7. nilnil [13] (v1.43.0 [12]) - Checks that there is no simultaneous
   return of `nil` error and an invalid value.
   This linter is disabled by default, but will be enabled for this
   template to prevent ambiguous returns.
8. tenv [14] (v1.43.0 [12]) - Detects using `os.Setenv` instead of
   `t.Setenv` since Go 1.17.
   This linter is disabled by default, but will be enabled for this
   template to prevent errors in tests.
9. contextcheck [15] (v1.43.0 [12]) - Checks if functions whether use a
   non-inherited context.
   This linter is disabled by default, but will be enabled for this
   template to broken call chains and loss of context information.
10. lll [18] - Add the `tab-width` option with value set to `2` to use
    two spaces for the width of one tab.
11. gci [19] - Add the comma-separated [list of prefixes for local
    package imports][20] to be put after 3rd-party packages.

[1]: https://github.com/golangci/golangci-lint/releases/tag/v1.43.0
[2]: https://github.com/ldez/tagliatelle
[3]: https://github.com/golangci/golangci-lint/releases/tag/v1.40.0
[4]: https://github.com/polyfloyd/go-errorlint
[5]: https://golangci-lint.run/usage/linters/#errorlint
[6]: https://github.com/tomarrell/wrapcheck
[7]: https://github.com/tomarrell/wrapcheck/releases/tag/v2.3.0
[8]: tomarrell/wrapcheck#2
[9]: https://github.com/golang/lint
[10]: https://github.com/golangci/golangci-lint/releases/tag/v1.40.1
[11]: https://github.com/breml/bidichk
[12]: https://github.com/golangci/golangci-lint/releases/tag/v1.43.0
[13]: https://github.com/Antonboom/nilnil
[14]: https://github.com/sivchari/tenv
[15]: https://github.com/sylvia7788/contextcheck
[16]: https://github.com/golangci/golangci-lint/releases/tag/v1.42.0
[17]: https://github.com/mvdan/gofumpt
[18]: https://github.com/walle/lll
[19]: https://github.com/daixiang0/gci
[20]: https://golangci-lint.run/usage/linters/#gci
[21]: https://github.com/mgechev/revive
[22]: #39

Closes GH-64
@tomarrell
Copy link
Owner

Thanks a lot for the feedback here. This issue has grown a little large and is still vague. I'm going to close it for now in favour of more specific issues which cover certain concrete additions.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
enhancement New feature or request
Projects
None yet
Development

No branches or pull requests

5 participants