Move errcheck package out of internal

[echlebek]

Allows using errcheck as a library. This will be more feasible now that we have versioning.

Signed-off-by: Eric Chlebek eric@sensu.io

[SVilgelm]

@kisielk @echlebek So, I have tested it with golangci-lint, everything works. Here is the PR: golangci/golangci-lint#1319

But I see a huge degradation in the performance due to loading the packages twice. @echlebek Could you split the CheckPackages function to two but moving this code: https://github.com/kisielk/errcheck/blob/master/internal/errcheck/errcheck.go#L241-L260 to a separate function. Or I can raise a PR to this branch with the changes

[kisielk]

@SVilgelm glad to hear it works. I think we'll be making some changes to the API as in the comments above, let me know what you think.

As far as the performance aspect, I think CheckPackages could probably be renamed to CheckPaths and then we could add a CheckPackage(*packages.Package) method would be called in that inner loop you referenced. Would that address the performance concerns?

[echlebek]

@dtcaciuc do you want to make a PR to this branch with the proposed changes?

[echlebek]

@SVilgelm @kisielk CheckPackage(*packages.Package) makes sense to me.

[SVilgelm]

hmm, I just found that you are using the *packages.Package when golangci-lint uses *types.Package, so need more time to figure out what to do with it

[SVilgelm]

found a workaround, @echlebek I will raise a pr to this branch with my changes

[SVilgelm]

@echlebek @kisielk Here is the PR for this branch: #188
I crate CheckPackage function and move some code from checkPaths to separated functions, like updating Ignore wit non verdure packages and implement isGoMod helper to use instead of go111module variable

[SVilgelm]

I understand, that you are going to change the configuration, but may I ask you to do it in separate pull request? That changes are backward incompatible and I'm not sure we can easy incorporate new config structure in golangci-lint, but will see

[kisielk]

I understand that, but I don't really want to mKe the API public and then immediately change it afterwards. Apart from naming it shouldn't be that different from the current one so I don't really see why it would be any more difficult to integrate.

[SVilgelm]

ok, if only naming, then probably we can keep old names in current release and update them in v2 of golangci-lint

Making the api public is not backward incompatible, but changing the config structure is

[echlebek]

I've merged @SVilgelm's PR to this branch. We can move forward from there.

I'd like to merge @dtcaciuc's branch next. I want to make sure I understand all the configuration changes fully first.

[echlebek]

I've merged Dimitri's PR and I've tagged this branch at v1.5.0-alpha.

The errcheck project now has a stable API, in the errcheck package, that can be used by linter aggregators. We don't expect to break this API until v2, once it is published as v1.5.0.

The errcheck package stutters a bit, because it is nested in the top level directory of the repository. Since errcheck was originally published as a tool, with package main in the repo root, we felt that it would be a breaking change to move the API to the repository root, even if it would result in a nicer looking import path.

The errcheck tool remains the same as before, with no functionality removed.

I've reviewed the godoc for the API and it looks fairly usable. More importantly, it is fairly minimal, which should make future expansions less troublesome.

[echlebek]

I've been doing a bit more work on this.

We may want to revisit the signature of CheckPackage. It returns []UncheckedErrors, but it should probably just return error, and expect the user to do a type assertion for more information. Any thoughts on this?

I'm about to make a PR to this branch that removes CheckPaths, and reimplement what it does in package main, using CheckPackage. This will shrink the API surface even more. Since CheckPaths goes away, LoadPackages appears, giving the API user an simple way to load a bunch of packages for errcheck checking.

[kisielk]

Probably would be better to make a Result struct that contains []UncheckedErrors and then change the return type to (Result, error). The error return could be used to return any actual errors from the checking process. Technically the failed checks are not "errors" in the usual sense of a Go program.

[echlebek]

I've encountered some difficulty with the tests while refactoring. They're tightly coupled to CheckPaths so they'll need some refactoring too.

[kisielk]

Maybe just a helper function for the tests would do the job?

[leventov]

Is there anything left to do before this PR could be merged?