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

report blacklist #132

Open
thumbnail opened this issue Feb 24, 2020 · 4 comments
Open

report blacklist #132

thumbnail opened this issue Feb 24, 2020 · 4 comments

Comments

@thumbnail
Copy link
Member

thumbnail commented Feb 24, 2020

Context

Sometimes a lint-warning won't need fixing (because edge cases, dev preference, etc.)
We want to have a way to ignore recurring warnings from bothering the user.

the CI integration (#88) would need this, so the build does not fail and the warning is considered unimportant.

Task

  1. create a extensible blacklist
  2. ignore reports matching the blacklist

possible approaches

  1. a simple edn list with the :line, :column, :filename, and :source of all (to be) ignored reports
    • con: if the location of the warning changes, the ignore list has to be updated
    • we can automatically check whether all ignores are still triggered.
  2. metadata, by tagging a form with ^:formatting-stack.<linter>/ignored.
    • moving the form doesn't break whitelist
    • self describing. it's right in the source.
    • con: clutters code with metadata irrelevant for the behavior of the program itself.
    • con: implementation might be more complex, as the violated form is not available in the reporter right now. Not all linters parse the source
  • con: not all reports address one specific form. e.g. loc-per-file, and one-resource-per-ns
@vemv
Copy link
Contributor

vemv commented Feb 24, 2020

If the CI integration was analog to https://github.com/DeLaGuardo/clojure-lint-action , namely applying a diff over a PR's changes only, maybe #132 wouldn't be that necessary.

As of lately, I'm thinking that a big CI step that analyzes the whole project is bound to be a nuisance to people, as always unrelated stuff will show up.

Requiring people to configure f-s seems not particularly attractive either.

(We can refine a https://github.com/DeLaGuardo/clojure-lint-action -like idea in a separate issue)

@thumbnail
Copy link
Member Author

If the CI integration was analog to https://github.com/DeLaGuardo/clojure-lint-action , namely applying a diff over a PR's changes only, maybe #132 wouldn't be that necessary.

This issue overarches the CI issue because also (new) devs shouldn't be bothered by acknowledged / allowed warnings anyway. The main goal of this issue is to address unnecessary noise during development on (big / legacy) projects.

'm thinking that a big CI step that analyzes the whole project is bound to be a nuisance to people, as always unrelated stuff will show up.

I don't think so. If we can create a good blacklist which doesn't require a lot of maintenance etc., we can prevent this issue altogether, and keep the CI integration simple (exit with (+ (count warnings)))

Requiring people to configure f-s seems not particularly attractive either.

The configuration would be additional, to allow more fine grained control over the output.

@vemv
Copy link
Contributor

vemv commented Apr 6, 2021

Given that by now, both clj-kondo and eastwood offer their own ignore 'syntaxes', f-s users can simply specify those and f-s will forward them to the underlying linters accordingly.

That might be sufficient to consider this issue solved?

There are more linters, but those don't emit false positives (as they tackle very tiny domains) so generally there's nothing to ignore.

@thumbnail
Copy link
Member Author

I think the priority is lower since the biggest linters ship with good ignore syntax nowadays. However I think a way to ignore built in linters is required before this issue can be closed

@thumbnail thumbnail mentioned this issue Sep 3, 2021
26 tasks
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

2 participants