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’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Comment imports to tolerate exceptions #6

Closed
nnutter opened this issue Feb 28, 2020 · 10 comments
Closed

Comment imports to tolerate exceptions #6

nnutter opened this issue Feb 28, 2020 · 10 comments

Comments

@nnutter
Copy link
Contributor

nnutter commented Feb 28, 2020

Imagine wanting to avoid a package but, in practice, needing to be able to tolerate exceptions to it (while using faillint in a CI pipeline). Much like many linting tools I wonder if the import could be annotated like,

import (
    "github.com/gogo/protobuf/proto" // faillint: gogo is tolerated here because XYZ.
)

So if we had -path "github.com/gogo/protobuf/proto=github.com/golang/protobuf/proto" but faillint saw a // faillint: annotation it would suppress its objection (and if no objections exit zero).

@nnutter
Copy link
Contributor Author

nnutter commented Feb 28, 2020

If this idea is acceptable I would be happy to explore if it's possible and put together a PR.

@fatih
Copy link
Owner

fatih commented Mar 6, 2020

Thanks nnuter. This makes sense, maybe there are some packages or files where people can't change and have to use certain packages. Do you know how other linters use this in their code? I think staticcheck also has this. We should try to use the same annotation pattern.

@nnutter
Copy link
Contributor Author

nnutter commented Mar 7, 2020

staticcheck uses:

  • //lint:ignore Check1[,Check2,...,CheckN] reason for line-based ignores
  • //lint:file-ignore Check1[,Check2,...,CheckN] reason for whole file ignores

Since faillint isn't checking multiple classes of problems it would probably make sense to use //lint:ignore reason and //lint:file-ignore reason. Another option might be to use faillint as a check name in the staticcheck style; staticcheck has codes like ST1019 for the check names.

I guess I'll explore this a bit since you seem interested.

nnutter added a commit to nnutter/faillint that referenced this issue Mar 7, 2020
Comment directive is based on staticcheck.

For: fatih#6
@nnutter
Copy link
Contributor Author

nnutter commented Mar 7, 2020

I created a PR. It was pretty easy to add. We can see what you think of the code, etc. Happy to iterate on any feedback or if you want to just take it and run with it that's fine with me too.

@bwplotka
Copy link
Contributor

bwplotka commented Mar 7, 2020

+1 to this request, but we might want to make it work for all declarations: #8 (:

@nnutter
Copy link
Contributor Author

nnutter commented Mar 7, 2020

I guess I'll wait until that PR is merged and then maybe take another pass at it.

@bwplotka
Copy link
Contributor

bwplotka commented Mar 7, 2020

Thanks!

@nnutter
Copy link
Contributor Author

nnutter commented Mar 7, 2020

New PR up.

@fatih
Copy link
Owner

fatih commented Mar 14, 2020

This is now closed with #11 Thanks again @nnutter on working on it.

Faillint v1.3.0 includes these changes.

@fatih fatih closed this as completed Mar 14, 2020
@bwplotka
Copy link
Contributor

💪

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

No branches or pull requests

3 participants