Closed ldez closed 1 year ago
Just so I am understand, This is more of an issue that NewAnalyzer
can potentially fail due to bad configuration passed in?
The goal with having it before the analyzer is created, please correct me if I am wrong, was to reduce the amount of computation needed to perform the analysis. compile
is only called once where Run
can be called many times over with different contexts passed in. compile
was supposed to be expensive so that Run
can just do its thing.
Briefly looking at golangci-lint
's WithContextSetter
, it would allow the compile to occur only once, right?
Would something like #51 do what is needed. That way golangci-lint doesn't have to modify the returned analyzer. Just call NewUncompiledAnalyzer
then use the Compile
method in the WithContextSetter
.
This is more of an issue that NewAnalyzer can potentially fail due to bad configuration passed in?
yes
The goal with having it before the analyzer is created, please correct me if I am wrong, was to reduce the amount of computation needed to perform the analysis.
No, it's not a way to reduce computation needed to perform the analysis but a way to not impact users that don't use depguard when an error occurs during compile
.
But in the end, the solution is the same.
Briefly looking at golangci-lint's WithContextSetter, it would allow the compile to occur only once, right?
yes and no: we need to call the NewAnalyzer
to have information about the linter, WithContextSetter
is only called when we run the linter. (build vs run)
WithContextSetter
is called only one time.
Would something like https://github.com/OpenPeeDeeP/depguard/pull/51 do what is needed.
Your solution is oriented on the protection of internals, it feels to me like a complex solution instead of just exposing the internal but it will work and it's easy to use.
v2.1.0 has been tagged with these changes. Thanks for the help and clearification!
Thank you for the quick changes and the release, I will open a PR on golangci-lint update to use the new version.
It's complex to explain but it's a simple problem.
Inside golangci-lint, we instantiate all the linters several times (one for the CLI help, one for the run, etc.) but because of the error reported by
compile
insideNewAnalyzer
, if an error occurs during the call toNewAnalyzer
then golangci-lint fails. This implies that an error can happen when a user callsgolangci-lint version
orgolangci-lint help
.I created a change in our fork to be able to control the error: https://github.com/golangci/depguard/commit/4f22f85857339ed5d88c4d44d7e52dae9c5b1444. You can see the use inside golangci-lint: https://github.com/golangci/golangci-lint/pull/3880/files
I don't want to keep this fork, can we think about a change to the lib API? The changes inside our fork are just a possibility, we can find other solutions.