Consider supporting //nolint:nilaway in standalone mode
#143
Comments
|
I'll also note that I'm happy to take a stab at contributing back support for this feature if the team's okay with it. I'll take a look at what was removed in #11, and try to add back the essential bits, without b0rking the integration story outside of standalone mode. |
When testing scenario filters with negations for variants that didn't exist for some scenarios, I ran into a panic in the Matrix implementation due to not checking for nil and handling it appropriately in our exclusions. After fixing it I decided to use the `nilaway`[0] static analysis tool to verify the entire `flightplan` package. The tool itself definitely has a few rough edges, and there were times when it was confused and I had to do things more than once in a function to please it, but overall it found several useful improvements. It was fairly time consuming so I scoped this to just the `flightplan` package for now. Eventually it might be nice to enable for the entire program but we'd either need to architect away our use of global variables for the UI, server client, and server, or we'd need them to reintroduce per line `//nolint` directives[1]. It also has a few show stopper bugs like not being able to handle listening on a nil channel in a select statement.[2] A tool with a promising and useful future but needs more time for us to enforce. [0] https://github.com/uber-go/nilaway [1] uber-go/nilaway#126 [2] uber-go/nilaway#143 Signed-off-by: Ryan Cragun <me@ryan.ec>
|
+1, right now many people can only run nilaway in standalone mode and removing |
When testing scenario filters with negations for variants that didn't exist for some scenarios, I ran into a panic in the Matrix implementation due to not checking for nil and handling it appropriately in our exclusions. After fixing it I decided to use the `nilaway`[0] static analysis tool to verify the entire `flightplan` package. The tool itself definitely has a few rough edges, and there were times when it was confused and I had to do things more than once in a function to please it, but overall it found several useful improvements. It was fairly time consuming so I scoped this to just the `flightplan` package for now. Eventually it might be nice to enable for the entire program but we'd either need to architect away our use of global variables for the UI, server client, and server, or we'd need them to reintroduce per line `//nolint` directives[1]. It also has a few show stopper bugs like not being able to handle listening on a nil channel in a select statement.[2] A tool with a promising and useful future but needs more time for us to enforce. [0] https://github.com/uber-go/nilaway [1] uber-go/nilaway#126 [2] uber-go/nilaway#143 Signed-off-by: Ryan Cragun <me@ryan.ec>
|
Original we had support for this Please note that the original support deviates from the actual meaning of Therefore, in #11 we removed such support to avoid the confusions, and more importantly, relies on the driver to implement such features (which is the case for nogo, and I think for golangci-lint?). Unfortunately, this doesn't seem to be the case for the driver in Anyways, I still think that such responsibilities should be on the driver side: linters should just throw whatever errors it finds, and it's up to the driver to decide what to deal with suppressions. But, unfortunately even with the standard Closing this as duplicated by #91 |
Background
In PR #11, the original hard-coded
//nolint:nilawayignore directive support was removed. This change allowed Nilaway to more easily integrate with other linter/analysis drivers, but also removed the ability for standalone Nilaway to ignore spurious errors.Waiting for other common linter drivers to integrate Nilaway may be futile-- see the consistently closed PRs and issues over in the
golangci-lintrepo, for instance. (The team there judged Nilaway to be too high a false positive rate for them to even consider integrating it as a supported linter. Using Nilaway as a plugin togolangci-lintis technically possible, but that style of integration is fairly unpleasant to implement and maintain, due to matching-version build requirements for bothgolangci-lintand the plugin.)Note: Teams using the Bazel
nogointegration may be able to use Nilaway a little-- I don't know anyone using Bazel for Golang development right now, so I can't comment on that integration.Proposal
If the standalone Nilaway driver under
cmd/nilawaycould have support for ignore directives again in some form, this would allow many teams and projects to try out the linter while it's maturing-- they will be able to work around the current high false-positive rate, and bring the linter into CI use cases more easily.The text was updated successfully, but these errors were encountered: