Configuration to define what is external

[gganley]

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]

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

[gganley]

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]

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 :beers:

[tomarrell]

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]

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

[gganley]

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]

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]

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]

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]

@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]

@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]

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]

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]

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]

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.

[tomarrell]

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.