Skip to content
This repository has been archived by the owner on May 9, 2021. It is now read-only.

Disable/Enable specific Rules #263

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

Disable/Enable specific Rules #263

steelbrain opened this issue Dec 1, 2016 · 59 comments

Comments

@steelbrain
Copy link

steelbrain commented Dec 1, 2016

Hi everyone 👋

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
Copy link

deitch 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
Copy link
Member

dsnet 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
Copy link

Zanndorin 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
Copy link

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
Copy link

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
Copy link
Member

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
Copy link

@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
Copy link

misham 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
Copy link

rana 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
Copy link

gaffo 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
Copy link
Member

dominikh commented Aug 1, 2017

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

@gaffo
Copy link

gaffo 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
Copy link

gaffo commented Aug 8, 2017

@dominikh ^?

@dominikh
Copy link
Member

dominikh 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
Copy link

gaffo 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
Copy link
Member

dsnet 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
Copy link

gaffo commented Aug 8, 2017

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

@gaffo
Copy link

gaffo 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
Copy link

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
Copy link

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.

@yvbeek
Copy link

yvbeek 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
Copy link
Member

andybons 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 as completed Feb 7, 2018
@mikijov
Copy link

mikijov 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
Copy link

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
Copy link

majelbstoat 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
Copy link

curtiszimmerman 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
Copy link

fgm 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.

@iddan
Copy link

iddan commented Feb 23, 2020

My workaround:

#!/bin/bash

# Lint all the files using go lint excluding errors which are currently explicitly ignore
# This script is intended to be used in the continuous integration process
# When editing, it is highly recommended to use ShellCheck (https://www.shellcheck.net/) to avoid common pitfalls

# Patterns to be ignored from the go lint output
IGNORED_PATTERNS=(
    " comment "
)

# Patterns joined into a regular expression
REGEX=$(printf "|(%s)" "${IGNORED_PATTERNS[@]}")
REGEX=${REGEX:1}

# Execute go lint on all the files and filter output by the regualr expression
output=$( ( (golint ./... | egrep -v "$REGEX") 2>&1 ) | tee /dev/fd/2);
if [ -z "$output" ]
then
    exit 0
else
    exit 1
fi

This can be done more robustly with Go but I didn't have the time to investigate how to do it.

@dgoodine
Copy link

My workaround: removed golint from our CI pipeline.

@dsnet's wariness about switches and knobs makes a fair point. Which is all the reason why rules need to be more clearly though out before they're established. The rule that using "ID" and not "Id" for an identifier is beyond the pale of neurotic style enforcement.

Tools that are otherwise useful but go beyond practical reasoning become more trouble than they're worth. And people will stop using them.

@RavenHursT
Copy link

@dgoodine Inevitable end to "linting"... #justLetFolksCode

@ghostsquad
Copy link

So it sounds like it's more important for the Go team to have a "all or nothing" stance than it is to make a useful tool?

I get that it's certainly possible (and currently being done) for the community to create their own tools. That's only saying that the standard library (of tools) is less adequate than what the community creates to replace them. You might as well not even create the tools if you don't care about real world usage.

The Zen of Python puts this nicely:

Special cases aren't special enough to break the rules.
Although practicality beats purity.

Pep 8 also has some wisdom

A Foolish Consistency is the Hobgoblin of Little Minds

A style guide is about consistency. Consistency with this style guide is important. Consistency within a project is more important. Consistency within one module or function is the most important.

However, know when to be inconsistent -- sometimes style guide recommendations just aren't applicable. When in doubt, use your best judgment. Look at other examples and decide what looks best. And don't hesitate to ask!

In particular: do not break backwards compatibility just to comply with this PEP!

Some other good reasons to ignore a particular guideline:

  1. When applying the guideline would make the code less readable, even for someone who is used to reading code that follows this PEP.
  2. To be consistent with surrounding code that also breaks it (maybe for historic reasons) -- although this is also an opportunity to clean up someone else's mess (in true XP style).
  3. Because the code in question predates the introduction of the guideline and there is no other reason to be modifying that code.
  4. When the code needs to remain compatible with older versions of Python that don't support the feature recommended by the style guide.

Specifically let me call out "use your best judgment". Accept that maybe your way of doing things doesn't work for everyone.

It's quite unfortunate that a programming language, which has a literal purpose allowing people to create things in the world that don't exist yet, or to modify the world around them, has such strict requirements around how it should be used. Even if you know better, it's also OK to let people learn and even fail.

@jcollum
Copy link

jcollum commented May 7, 2020

So it sounds like it's more important for the Go team to have a "all or nothing" stance than it is to make a useful tool?

The Go ecosystem is usually a "do it the Go way or go away" kind of thing.

This thread only reinforces my conclusion that the bigwigs in the Go community have way too much intellectual rigidity. All this churn because some people want to ignore some lines during linting? ... Really?

@ianlancetaylor
Copy link

The many Go developers have tried to develop a framework for tooling that can be used by tools like lint. There is also a specific lint tool that uses that tooling and tests various style guidelines, as outlined at https://go.googlesource.com/lint/+/refs/heads/master/README.md. If you want a different tool that meets different goals, you can use use the tooling framework to write your own tool.

To put it another way, there is one tool that the Go developers feel should be used by everyone. That tool is go vet. There is also this lint tool. Nobody thinks that this tool should be used by everyone. It's just a convenient tool for some people, and an example tool for people who want something similar but not exactly the same.

@ghostsquad
Copy link

I just find it a bit ridiculous that practical reasons, of which there are many described in this thread, are dismissed as unnecessary knobs. That's like telling a aircraft pilot, that his control plane has too many knobs, and that he should use an iPhone (with just one button) instead.

@ianlancetaylor
Copy link

That's a good analogy. I would say that the lint tool is an iPhone, and you want an aircraft. There is a role for both iPhones and aircraft in this world. We can help you build your aircraft if you want. It just won't be the lint tool.

(There are several other lint tools available already, and perhaps one of them is more to your liking. For example, you might want to look at https://pkg.go.dev/honnef.co/go/tools/staticcheck?tab=doc).

@jcollum
Copy link

jcollum commented May 8, 2020

Thanks for giving us the lay of the land there @ianlancetaylor

@jacobeatsspam
Copy link

I get that tools not run through go (e.g. go vet) are not considered first class citizens, but I thought this discussion process was the means by which tools become productionalized. Closing this issue is effectively saying this is a pet project and nothing more.

@ianlancetaylor
Copy link

No, this is a way of saying that this specific project has specific goals, and is not intended to be all things to all people.

@ghostsquad
Copy link

@ianlancetaylor we aren't asking it to be all things to all people. We are asking to be able to fill the fuel tank ourselves, instead of requiring someone else to fill it. We are asking for Visual Flight Rules & Instrumental Flight Rules (piloting), because the real world has weather and low visibility.

We are not asking for it to be complicated. Simple is good. But without knobs, it's like driving a car without:

  1. Radio (silence is superior)
  2. A/C (just roll down your window)
  3. GPS (just remember the route and plan before hand)

If you feel strongly about the rules of linting, make them default, but allow them to be configurable. That's all anyone here is asking for.

Anyways... I don't think this is productive anymore. Frankly, this type of thread just says a lot about how the team doesn't believe that Practicality beats Purity.

@ianlancetaylor
Copy link

Use the underlying tooling to do whatever you like. Or just copy the source code of this project and change it however you like. Perhaps your new project will become more popular than this one, and we can retire this one.

This project has a defined set of goals, as mentioned above. There is nothing wrong with having different goals. The Go project wants to give you the tools to meet those goals. Where we haven't given you those tools, please let us know.

But this particular project is meant to support a specific defined set of goals. It is not meant to be a general purpose program. It is not meant to be used by people who do not share the goals of this program.

Perhaps we just need to rename this project. Perhaps people see "lint" and think that it is something they should use. For Go, the thing you should use it called "vet". You should only be using this project, "lint", if you think it has good goals. If you don't, you should be using a different project. There are many tools out there for linting and checking Go code. There is nothing special about this one.

@jacobeatsspam
Copy link

jacobeatsspam commented May 8, 2020

Honestly, I just read this as you asserting your beliefs. This doesn't match anything I've ever heard from talks by core contributors. Go is improved through rigorous debate, and you're arguing against discussion at all. I'd rather hear this from a maintainer.

If you feel things are fine, why even comment on a closed issue? Just unsubscribe if you don't have the power to meaningfully weigh in.

@ianlancetaylor
Copy link

It's not clear to me why you think that I am arguing against discussion. There is plenty of discussion here. I haven't said that it should stop.

A decision was made on this issue back in #263 (comment). More discussion here isn't going to change that decision. But by all means continue to discuss.

I'm commenting here because I see people expressing unhappiness and dissatisfaction with the Go project, and I'm trying to explain why the decision is what it is, and I'm trying to point out the other options that people have to achieve their goals.

@ghostsquad
Copy link

ghostsquad commented May 8, 2020

The Go project wants to give you the tools to meet those goals. Where we haven't given you those tools, please let us know.

See this thread...

Perhaps we just need to rename this project. Perhaps people see "lint" and think that it is something they should use. For Go, the thing you should use it called "vet". You should only be using this project, "lint", if you think it has good goals. If you don't, you should be using a different project. There are many tools out there for linting and checking Go code. There is nothing special about this one.

Perhaps. I think the problem here is that once a tool is made "by the official team", people tend to latch on to it, and prefer it over other homegrown tools. This is just my perception. Sometimes those "official tools" don't meet the standards and requirements of some, and community built tools do become more popular. Let's look at history to see how that pans out:

  1. pkg/errors -> Go Team realized that errors are too simple, and some additional knobs are needed
  2. dependency management -> deb, glide, etc are invented -> Go Team realizes that "just use the HEAD of master" is not as practical, and some additional knobs are needed, and we land on go modules.
  3. generics -> cheekybits/genny is invented, because requiring every developer to write code generation themselves is actually really burdensome -> hopefully a generics proposal is accepted soon and the compiler can do the code generation.

Please don't misinterpret this as me being against "start simple and iterate". Indeed, I am all for starting simple and iterating.

@adamrbennett
Copy link

If I write a tool for my personal use and the community doesn't like it I would respond in the same way the maintainers have responded here.

If I write a tool to support a community and it doesn't fit their reasonable needs, I would alter it.

Is this tool built for the community or for the maintainers?

On the topic of project goals: is one of the goals of this project to support the reasonable needs of the community?

@ghostsquad
Copy link

Is this tool built for the community or for the maintainers?

On the topic of project goals: is one of the goals of this project to support the reasonable needs of the community?

I definitely think these are super relevant question here.

@ianlancetaylor
Copy link

Those are good questions.

I have opened a proposal at https://golang.org/issue/38968 to simply remove the lint program. See that issue for my reasoning.

I encourage people to comment on that issue, preferably about the meta-topic of whether the lint program should exist or not rather than details of what the lint program should do.

Thanks for the comments.

@ghostsquad
Copy link

I have opened a proposal at golang.org/issue/38968 to simply remove the lint program. See that issue for my reasoning.

Upvoted

@dgoodine
Copy link

dgoodine commented May 9, 2020

Upvoted. Thank you @ianlancetaylor.

@ericraio
Copy link

Upvoted thanks @ianlancetaylor

@phanirithvij
Copy link

phanirithvij commented Nov 2, 2020

It's similar with the dartfmt maintainers another great framework (Flutter) built for the community, by our beloved Google.

@patarapolw
Copy link

A use case is, not-yet-fixed golint issues...

#393

Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
None yet
Projects
None yet
Development

No branches or pull requests