Create "warn only" functionality in flake8. - [closed] #1183

Closed asottile closed 3 years ago

In GitLab by @scolby33 on May 28, 2018, 18:40

Merges add-warning-functionality -> master

Add the --warn-only and --extend-warn-only options analogous to the --ignore and --extend-ignore options.

Extend the checker and style_guide to make use of the new "warn only" options.

See issue #424

Edit: see issue #332 as well

In GitLab by @scolby33 on May 28, 2018, 18:47

added 1 commit

b565ffbb - Small fixes for linting/style.

Compare with previous version

In GitLab by @scolby33 on May 28, 2018, 18:48

If the general consensus on this approach is okay, I will proceed to documentation. Otherwise, I'm open to thoughts on my work!

In GitLab by @scolby33 on Oct 21, 2018, 24:15

added 23 commits

b565ffbb...7801a17a - 21 commits from branch pycqa:master
01e60bd5 - Create "warn only" functionality in flake8.
910e3aad - Fixes for linting/style.

Compare with previous version

In GitLab by @codecov on Oct 21, 2018, 24:16

Codecov Report

Merging #238 into master will decrease coverage by 0.11%. The diff coverage is 73.33%.

@@            Coverage Diff             @@
##           master     #238      +/-   ##
==========================================
- Coverage   80.21%   80.09%   -0.12%     
==========================================
  Files          25       25              
  Lines        2355     2381      +26     
  Branches      390      394       +4     
==========================================
+ Hits         1889     1907      +18     
- Misses        385      389       +4     
- Partials       81       85       +4

Impacted Files	Coverage Δ
src/flake8/main/options.py	`100% <100%> (ø)`	:arrow_up:
src/flake8/style_guide.py	`95.83% <66.66%> (-2.66%)`	:arrow_down:
src/flake8/checker.py	`75.56% <80%> (-0.18%)`	:arrow_down:

Continue to review full report at Codecov.

Legend - Click here to learn more Δ = absolute <relative> (impact), ø = not affected, ? = missing data Powered by Codecov. Last update 98beabd...85ced38. Read the comment docs.

In GitLab by @scolby33 on Oct 21, 2018, 24:19

changed the description

In GitLab by @scolby33 on Oct 21, 2018, 24:26

Rebased to resolve conflicts.

In GitLab by @scolby33 on Apr 15, 2019, 11:05

added 137 commits

910e3aad...98beabdc - 135 commits from branch pycqa:master
0bed1329 - Create "warn only" functionality in flake8.
85ced38b - Fixes for linting/style.

Compare with previous version

In GitLab by @scolby33 on Apr 15, 2019, 11:06

Rebased again to resolve conflicts.

In GitLab by @asottile on Apr 15, 2019, 11:12

I'm biased against warning noise, especially in linters. You ~could already do this without needing an option by using flake8 --select ... || true.

In GitLab by @scolby33 on Apr 15, 2019, 11:25

That's true and I hadn't considered it before.

In my notional tox.ini I have something like flake8 --select $IMPORTANT_ONES --warn-only $WARNING_ONES. With your alternative, I need flake8 --select $IMPORTANT_ONES --ignore $WARNING_ONES; flake8 --select $WARNING_ONES --ignore $IMPORTANT_ONES || true.

I kinda hated this until I had to type out my example here and realized it's not so bad if I actually use variables of some form; originally I worried about keeping the lists in the two commands in sync.

As a counterpoint to avoiding warning noise, this is only opt-in warning noise. My intention for use of this feature was in adding flake8 to an old codebase. Warn on everything until it passes, then start failing, thus preventing the whole test suite from turning red and allowing incremental improvements. Perhaps this is misguided and the approach should be to enable all the warnings you'd want and start checking on a file-by-file basis instead of warning-by-warning?

Anyway, I think this is potentially useful and it would solve a pain point I've encountered before. However, if it is not in the cards to be added, let's close this merge request and also #332. If there's a relevant portion of the documentation to add the flake8 --select || true solution as a recipe/example, I'd be happy to add it.

(One thing I realized as I typed this last part, the || true solution requires a "sane" shell for some definition of sane. It wouldn't be cross-platform if the tests were run under fish, for example, and would require extra logic if flake8 is called as a subprocess from another program, whereas just sticking with 0/not 0 return codes fits with existing logic and tooling.)

This was longer than I expected it to be. Let me know what you think!

In GitLab by @asottile on Apr 15, 2019, 11:40

Actually, even better than || true is to use --exit-zero which is already implemented :D

In GitLab by @jcgoble3 on Apr 15, 2019, 11:47

See issue #332. Part of the idea of this is that a single CI run can simultaneously fail on actual problems, while nagging about known code smells without failing on them until people find time to fix them. Currently doing this requires two CI jobs, one for actual failures and one with --exit-zero to report the nagging. That clutters the CI interface and wastes time and resources to spin up the second machine. This would enable both to be accomplished in a single job for better efficiency.

In GitLab by @jcgoble3 on Apr 15, 2019, 11:49

And the alternative to a second CI job now is to just straight skip and ignore all the known code smells, which only hides them and accumulates technical debt.

In GitLab by @asottile on Apr 15, 2019, 12:34

Couldn't you have a single CI job which just calls two things? (think: tox environment with two commands in it)

[testenv:flake8]
commands =
    flake8 --select=...
    flake8 --select=... --exit-zero

In GitLab by @jcgoble3 on Apr 15, 2019, 18:19

That would work in theory, but is there a way to maintain the two lists simultaneously in a config file (other than as explicit arguments to a command)? I prefer to set all options in config files and invoke testing commands with as few arguments as possible (preferably none at all) for easier maintenance and easier integration with my IDE.

With Visual Studio Code, relying on config files means that I can change just the config file and it is immediately picked up by both Code and my CI, while editing command line arguments requires editing two files, specifically the CI config and the workspace settings, violating the "single source of truth" principle.

Since both lists are being passed to --select=... here, I don't see off the top of my head how that would be possible, plus an explicit --exit-zero argument appears to still be needed even if it is. Having a separate argument for specific code smells to report but not fail on would mean that both lists can be together in one config file, allowing the invocation to be nothing more than a bare flake8.

In GitLab by @asottile on Apr 15, 2019, 20:50

I'm not convinced the complexity is worth it -- there's already a way to do what you want -- yeah it takes two commands, but enabling this (in my opinion anti-feature of warning noise) is not worth it.

tox is a config file that could have a single source of truth, can you point your editor at that? Or even a shell script?

In GitLab by @jcgoble3 on Apr 15, 2019, 21:14

I'm not convinced the complexity is worth it -- there's already a way to do what you want -- yeah it takes two commands, but enabling this (in my opinion anti-feature of warning noise) is not worth it.

Strictly opt-in warning noise. Those that don't use it shouldn't see any change. Only those that want the noise will get the noise. And given the concept of nagging people until code smells get fixed, I'd argue that the noise is a good thing.

tox is a config file that could have a single source of truth, can you point your editor at that? Or even a shell script?

For unit tests, of course. However, Code has a separate mechanism for linting. The linting mechanism does not support arbitrary shell commands; it expects to invoke one of a small number of known linters, such as flake8, exactly once and get an output list (possibly empty) of warnings and errors, which is then parsed to identify and highlight those problems directly in the editor. Command-line arguments, if any, to the linter invocation must be configured separately as a variable within the user or workspace settings.

Hence my desire to avoid any and all arguments, keeping everything in one place (such as a tox config file), and to be able to call a bare flake8 and get exactly what I want. If I use arguments to the flake8 call, I must break the single source of truth and specify those arguments twice: once in whatever config file I use (CI, tox, etc.), and again in Code's workspace settings so the code highlighter can run correctly.

The two-invocation concept fails in two ways: it limits what I have access to inside Code (which cannot accumulate notes from multiple linter runs), and forces me to use command-line arguments since both commands use different values for the same argument (--select).

I would imagine that if this is merged and released, the Python extension for Code would eventually grow a checkbox option to enable/disable highlighting of the "nag" items (assuming it is possible to parse them as such, or possible to programmatically turn them off), so they can be ignored while working on more important things and enabled when fixing them. I would probably file the feature request for that myself.

In GitLab by @asottile on Apr 15, 2019, 21:22

Strictly opt-in warning noise. Those that don't use it shouldn't see any change. Only those that want the noise will get the noise. And given the concept of nagging people until code smells get fixed, I'd argue that the noise is a good thing.

From experience, warning noise causes developers to ignore the entire output instead which is way worse. It also creates fatigue with the tool in general

Can you customize the command the VS Code extension runs? I see no reason you can't have your own flake8 executable which just calls flake8 --select=...; flake8 --select=... --exit-zero

And again, I get that you want your convenience, but we're weighing your convenience against the maintenance cost of one of the more complex parts of flake8 -- furthering the combinatorial explosion of --ignore / --select / (implicitly selected / implicitly deselected) / --enable-extension that's so complicated today I have to read the tests to understand how it works and explain it!

Not to even begin to mention the user experience here -- there's no differentiation in the output into what's fatal vs warning.

In GitLab by @sigmavirus24 on Apr 16, 2019, 08:19

My intention for use of this feature was in adding flake8 to an old codebase. Warn on everything until it passes, then start failing, thus preventing the whole test suite from turning red and allowing incremental improvements.

I've done this in the past with --exit-zero and a separate --diff that fails if any violations are introduced in the diff. It means that you should be making constant progress towards just running flake8 against everything without having to add more complexity to the mental model the Flake8 userbase has to understand when thinking about violations that can be:

ignored through flake8's default ignore list
ignored because the extension itself wants it ignored by default
ignored explicitly through one of a bunch of potential configuration files and command-line arguments
ignored on a per-file basis such that the style of the entire code-base is entirely opaque to any new-comer
enabled extensions that can have default ignored and default selected codes
selected by default (by which I mean, there is no default selected list so they're selected because they don't exist in the default ignore list)
selected explicitly through one of a bunch of potential configuration files and command-line arguments

And adding this would be "Things that are shown but don't cause errors causing genuine confusion about what needs to be fixed versus what should be fixed versus something else".

While I accept that y'all think this serves a purpose in a particularly useful way, I have to say that I really don't want the added complexity in here for the users of Flake8. And I haven't even looked at what this does to the codebase so I won't comment on the complexity it introduces there.

In GitLab by @scolby33 on Apr 16, 2019, 09:09

This thread has been a goldmine for learning options--I had never looked at --diff before.

As for the codebase complexity, I can't say I'm thrilled about how I added the functionality, which is a vote against merging this that I totally understand.

In GitLab by @asottile on Jun 14, 2019, 11:13

Thanks again for the discussion and PR -- given the resolution above and the existing approaches to accomplish the same goals I'm going to close this

In GitLab by @asottile on Jun 14, 2019, 11:13

closed

In GitLab by @con-f-use on Sep 5, 2019, 13:41

Suggestion: put the summary of this thread in the FAQ.

I'd also say that the warn-only and extend-warn-only give a more fine grained control over what's important and warning noise, is only noise if there is a lot.

PyCQA / flake8

Create "warn only" functionality in flake8. - [closed] #1183

Codecov Report