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鈥檒l occasionally send you account related emails.

Already on GitHub? Sign in to your account

Disable/Enable specific Rules #263

Closed
steelbrain opened this issue Dec 1, 2016 · 32 comments

Comments

Projects
None yet
@steelbrain
Copy link

commented Dec 1, 2016

Hi everyone 馃憢

I've performed a github issues search before opening this issue and couldn't find an already existing one. golint is great but sometimes you want all the rules except one and it would be wonderful to specify it somewhere. Other modern linters like ESLint support it, would be good to have it supported in golint as well.

@deitch

This comment has been minimized.

Copy link

commented Dec 20, 2016

Been looking for exactly that. E.g. I might not have proper docs on an exported function, but for a given project but might not matter.

@dsnet

This comment has been minimized.

Copy link
Member

commented Dec 28, 2016

Hi, I'm on the fence about whether this should be done. In the mean time, I would like to gather more information about why this should be a feature.

My thoughts are:

  • The purpose of lint is to encourage a consistent style of Go across the community. An individual developer may not agree with its style rules (I don't agree with some of them), but there is a strong benefit to consistency in the Go community. The ability to turn on/off specific rules is a very slippery slope where a person can disable a sufficient number of rules such that the tool can't effectively do its job.
  • There are certain rules that lead to many false positives (e.g., it flags something as a style violation that most people would agree is okay). These cases can be annoying, but I would vote that we either tighten the rule or delete these rules than a blanket ability to enable/disable rules.
  • Why is the min_confidence flag not sufficient for your use case? Are the confidence values badly chosen and don't fit your use-cases?
@Zanndorin

This comment has been minimized.

Copy link

commented Mar 8, 2017

@dsnet There is the case that I might have some legacy code which I cannot refactor for reasons of time.

But the real reason I'm here is because I wanted to turn off the warning of not commenting public functions in the internal legacy application im coding in.

@gonzaloserrano

This comment has been minimized.

Copy link

commented Apr 10, 2017

I'm with @Zanndorin here, I want to disable the exported function XXX should have comment or be unexported check since I'm dealing with legacy code.

@StephenWeatherford

This comment has been minimized.

Copy link

commented Apr 22, 2017

And what if the community has an established code that does not follow some of the rules? I'm working on an open source project which has about 1500 warnings about not using underlines (and underlines are used because it most closely matches their data model - and they have a confidence of 0.9). It's unreasonable to expect all communities to have the exact same set of rules in all situations.

@dominikh

This comment has been minimized.

Copy link
Member

commented Apr 22, 2017

It's equally unreasonable to expect golint, which was written for a specific style of Go, to accomodate the needs of all communities that feel it necessary to deviate from that style.

You're free to fork golint and maintain a version specific to your community.

@StephenWeatherford

This comment has been minimized.

Copy link

commented Apr 22, 2017

@dsnet specifically asked why the feature was needed, and why min_confidence isn't sufficient for particular use cases. So I'm answering that question as requested. Having more flexible options would make the tool more useful in general.

@misham

This comment has been minimized.

Copy link

commented Jun 14, 2017

It's helpful to omit a rule in on a single line while preserving it for other cases in a project. For example, when implementing callback functions for a library I don't need to add documentation to it or I'm dealing with legacy code.

It would be useful to be able to "turn off" a warning for a specific line by adding a comment, similar to how other linters behave. This way, there is a comment in the code that a lint warning is being ignored and why. The development team can agree or disagree with this as part of a code review.

By being able to control linter's checks in this manner, I can wire it up to a Makefile or a CI script and have it run on every commit without worrying about false positives or instances where I just don't care enough to fix it.

@rana

This comment has been minimized.

Copy link

commented Jul 7, 2017

Linters are suggestions, not rules. I do observe the linters suggestions, but I sometimes disagree, such as using underscores can be helpful in some cases. Rules are enforced by the compiler and gofmt. Everything else is legal and optional; therefore, it would be nice to disabled some lint suggestions. The challenge is that if i don't conform to every golint suggestion the warnings become too numerous, ignored and I abandon the linter.

@gaffo

This comment has been minimized.

Copy link

commented Aug 1, 2017

Another example I've hit recently is that the linter tells me that fields need to be named in a certain way, like Id should be ID, or no underscores in names.... however I'm working with other systems which have the opposite rules and already specified the protocol and naming so in this case these rules are just generating noise for me. If I was working in an entirely go world then I'd be totally fine with everything following the go rules but I'm working in an existing ecosystem and that's not likely to change. Therefore I have the option of either using the linter and ignoring tons of things with tons of grep -v in my make file and some non-intuitive scripting to see if the linter returned an error that I cared about so I can fail the build... or I can just turn off the linter. I'd rather have a linter that let me use as much as possible of the rules.

@dominikh

This comment has been minimized.

Copy link
Member

commented Aug 1, 2017

Third option: fork the tool, implement the rules of your organisation.

@gaffo

This comment has been minimized.

Copy link

commented Aug 1, 2017

How open would you be to changes that make forks a bit easier but don't implement the feature in the core?

@gaffo

This comment has been minimized.

Copy link

commented Aug 8, 2017

@dominikh

This comment has been minimized.

Copy link
Member

commented Aug 8, 2017

@gaffo that's really a question for @dsnet, the maintainer of golint. But you'd also have to be more specific than "changes". What specific changes would make forks easier?

@gaffo

This comment has been minimized.

Copy link

commented Aug 8, 2017

I was just checking if you guys were antithetical to them. It likely depends on how the main is structured if I can get a list of issues back before they're printed out then it'd be easy to do. if that's all tied up in main I'd likely just want to decouple the main from the error scanner library style.

@dsnet

This comment has been minimized.

Copy link
Member

commented Aug 8, 2017

I may have been the last person to do significant work on this repo, but I don't own this. In fact, I don't think lint has a clear owner. For this reason, I don't believe that I am the right person to make calls like this.

That being said, I am ambivalent about this feature. I can see how it helps people since some warnings are really annoying (and my opinion is that they should just be removed), but I see it as a slippery slope also to effectively make lint not enforce anything. In my personal projects, I actually disable the "should have comment or be unexported" rule all the time by just passing the output of lint to grep -v.

If we were add the ability to disable rules, I like the approach that staticcheck takes, which involves a list of "FILE:RULE" like "github.com/dsnet/compress/brotli/*.go:SA4016", where you have to be explicit about what rule you are disabling and where. However, that presumes that the rules in lint have an ID number, which they don't. We could always add them.

\cc @alandonovan, can we find an actual owner to make decisions on issues like this?

@gaffo

This comment has been minimized.

Copy link

commented Aug 8, 2017

Okay... that's enough willingness to think about it that I'll look into it.

@gaffo

This comment has been minimized.

Copy link

commented Aug 8, 2017

The main does return a list of Problems tho which is what I think I need for a fork I believe

@jnicholls

This comment has been minimized.

Copy link

commented Aug 18, 2017

I agree on the ability to disable certain lints. Ironically my use case is also the uncommented exported symbols, like a lot of others on this thread.

@jnicholls

This comment has been minimized.

Copy link

commented Aug 18, 2017

On that note, I'm currently instead using gometalinter and using the --exclude regexp capability to ignore these specific warnings. This is working well, and may be useful for others on this thread.

@Zyphrax

This comment has been minimized.

Copy link

commented Jan 31, 2018

You should be able to disable certain rules. This is a linter, not a compiler, it helps people to use a common style, but rules shouldn't be set in stone.

I think that by not offering the option to disable rules, people will likely disable the linter when it keeps nagging them, resulting in a lot of bad code.

It is like saying you can't use a screwdriver to open a paint bucket... why not let the user decide?

@andybons

This comment has been minimized.

Copy link
Member

commented Feb 7, 2018

From https://github.com/golang/lint#purpose (emphasis mine):

The suggestions made by golint are exactly that: suggestions. Golint is not perfect, and has both false positives and false negatives. Do not treat its output as a gold standard. We will not be adding pragmas or other knobs to suppress specific warnings, so do not expect or require code to be completely "lint-free". In short, this tool is not, and will never be, trustworthy enough for its suggestions to be enforced automatically, for example as part of a build process.

There are other linters being actively maintained by the community that serve this use case.

@andybons andybons closed this Feb 7, 2018

@mikijov

This comment has been minimized.

Copy link

commented Feb 10, 2018

I have put together a quick hack for -disable_category and -disable_check options. I am not making it a PR just yet as there does not seem to be willingness for this direction. If there is willingness, I can further improve the code. Please see my fork if you find it useful. Comments are in README.md.
https://github.com/mikijov/lint

@frankgreco

This comment has been minimized.

Copy link

commented Mar 9, 2018

While I completely agree that all go projects should conform to the statement in the purpose of golint, there are use cases where you're using a dep that doesn't conform to it and hence golint will complain even though the change is out of your control.

@majelbstoat

This comment has been minimized.

Copy link

commented Mar 27, 2018

FWIW, a specific use case for this is implementing a GRPC server that has an autogenerated method like GetUserById in it, rather than GetUserByID. The linter doesn't know the method needs to be called that to conform to the correct interface. And the go protobuf folks have declined to modify the autogenerated code to follow Go naming conventions stating that 'autogenerated code should not be held to the same high standards as human code': golang/protobuf#156

So, yeah, caught between two conflicting world views. If anyone else finds themselves in this position, I moved to using gometalinter and did // nolint: golint on the affected lines.

@curtiszimmerman

This comment has been minimized.

Copy link

commented Apr 2, 2018

Following the advice here for using gometalinter worked out really well and cost very little time to install and configure. And it turns out that using gometalinter and directives to ignore the stylistic suggestions from golint actually opens up a lot of power for linting that golint by itself can't hope to compete with.

@fgm

This comment has been minimized.

Copy link

commented Apr 30, 2018

Similar need here: on specific cases it is useful to specify the type of a variable, which might be omitted (anonymous functions), but leaving it in place allows easier refactorings by finding all instances of the type, so I would like to have a way to disable linting for the next line.

This seems less intrusive / slippery than disabling a rule entirely. And, with the experience of JS linters, the recent success of Prettier seems to imply keeping configurability to a minimum is a good idea.

@adamrbennett

This comment has been minimized.

Copy link

commented Feb 7, 2019

I'll add that the more warnings we have to ignore, the less useful the linter becomes. The example given above of 1500 linter warnings is a good example. Under this scenario, a new warning will easily be missed and the linter has failed to do its job -- which is not to simply show the warnings, but to encourage proper style, which is dependent upon the visibility and prominence of warnings. Too many warnings decreases visibility.

I understand the motivation to resist a lint config. It allows for too broad of a deviation from go style standards. However, I think line item disabling via comments would be a nice compromise.

@tnm-

This comment has been minimized.

Copy link

commented Mar 22, 2019

@adamrbennett Exactly. I enabled lint, got over a 100 warnings, a handful of them was useful. I wanted to automatically ignore the rest. Since it's not possible, I'm gonna disable lint as it is now 95+% noise.
I think it would have been OK, to add the most simple workaround, e.g. adding a special comment like //lintskip to an "offending" line.

@borud

This comment has been minimized.

Copy link

commented Jun 3, 2019

I think this is a principle versus practicality discussion. I would prefer to run the linter continually while I develop to catch any mistakes, omissions and typos. This is useful as it flags problems as you are writing code and not later. This isn't practical if the IDE constantly flags code as problematic when it isn't. I could change the code, but quite frankly, in some cases you do want to be able to have non-public types with public functions. Of course I could skip every file where the linter throws up suggestions, but then what would be the point?

The tool simply doesn't work for my regular test-vet-lint-build cycle and I've stopped using it on most projects. I don't understand what is so important about not implementing a "nolint" comment or an option that allows you to filter out certain rules when so many users appear to have the same problem.

And it isn't a simple matter of forking the linter or using a different one. I'm not interested in spending time wondering what linter to use. I'm not interested in having to re-check every now and then if the linter is being maintained.

And no, the -min_confidence flag is not an attractice solution because then you burden the user with the task of having to figure out what other problems are NOT being listed. And perhaps you do want to see some of those suggestions that get filtered out.

@shellscape

This comment has been minimized.

Copy link

commented Jun 10, 2019

The ratio of reactions here would suggest the maintainer(s) are vastly outnumbered and disagreed with by the the community the tool is supposed to support.

@Waitak

This comment has been minimized.

Copy link

commented Jun 11, 2019

For me, the issue here is that some suggestions are based on incorrect assumptions that the linter makes. The "stutter" warning is a case in point for us. I have the following types:

type SCACQueryParams struct { ... }
type SCACsQueryParams struct { ... }

These are used to call functions called GetSCAC() and GetSCACs(), respectively. The first generates the warning:

type name will be used as scac.SCACQueryParams by other packages, and that stutters; consider calling this QueryParams.

You can see the thought behind the warning, but it doesn't apply here. The suggestion, in fact, would be a terrible idea. I'd love to be able to tell the linter "Okay, I see your point, but there's a good reason why that doesn't apply here. Stand down on this one."

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
You can鈥檛 perform that action at this time.