runner now skips unselected visitors

[jakkdl]

also added --select pytest option, fixed code coverage & tests, renamed ARGS to ARG and generated required type stubs.

Relatively large PR, but other than the generated type stubs the individual changes should be relatively contained and understandable and/or commented.

I started using test skipping when fixing trio200, and after digging into flake8 when debugging the config errors I thought this shouldn't be too hard. It turned out to be quite a pain to get working properly, but the runner will now decide which visitors to include depending on flake8's parsing of a users --select,--ignore,--extend-select,etc etc - and the tests now also only selectively enables visitors instead of throwing out non-matching errors. This led to a speedup in testing, tox -e py311_flake8_6 dropped from ~7s to ~5.5s, and it also revealed several minor coverage issues. Those code paths were visited previously, but in checks intended for other errors, so had they raised errors or not worked correctly it would've been ignored.

Also now if a visitor starts crashing, it should be easy for an end-user to disable it for debugging and use the plugin until it's fixed without having to modify any code.

The internal functions in flake8 I'm relying on aren't publicly documented afaik, though it would be very nice with a stable API that exposed DecisionEngine.decision_for, so I wouldn't be shocked if those parts of the code broke at some point in the future - but modifying the runner to always include all visitors should be trivial.

The fuzz tests were also hella broken, since they didn't actually consume the iterator from .run(). But they now both work and will now properly crash if the code is broken. I didn't bother figuring out how to get unittest to play along with pytest options, so for now the code generator does not care about the added --select pytest option. The other one I could easily move outside the unittest class, and looking at runtimes it now seems to take about 20s for any single enabled visitor, with 60s if all visitors are enabled, but I couldn't see any difference from enabling/disabling singular visitors large enough to stand out from the noise.

I renamed ARGS to ARG to reflect actual use and that you can specify it multiple times. Though you can also specify multiple flags on one line if you want to.
And # INCLUDE is no longer needed as that can be served just as well with # ARG --select=...

test_decorator.test_plugin I'm pretty sure has become redundant and was a pain to update, so I scrapped it.

Adding --select meant I could simplify a bunch of the test infrastructure, and test_noerror_on_sync_code is now way cleaner when it can just specify --ignore for the ignored error codes. But needed some restructuring to call initialize_options in several places.

[Zac-HD]

I read through and this looks good - and the speedup will be nice, at least for testing where we don't just always enable everything 😅

My main concern is that users of the lovely flake8-noqa plugin might be checking whether there are unused ignores, and disabling the checker would have exactly that effect. Can we avoid disabling checkers if flake8-noqa is available, and ideally only then if NQA10{1,2,3} is selected?

[jakkdl]

hmm, I don't think #noqa's will disable visitors as that's a per-file check and this is part of the initialization and just checks config and command-line options. Whereas flake8 suppresses output from noqad lines at a later stage. But I haven't tried it

[Zac-HD]

Let's suppose that I have some file that has a # noqa'd trio200 error. If I then run flake8 --select=TRIO102,NQA:

1. flake8-trio will run some checkers and emit a sequence of errors to flake8
2. flake8-noqa will check that each noqa comment correctly suppresses an error. Unfortunately, our deselection logic operates before this check, so it looks like the noqa comment is unused! (well, it is unused in this configuration)
3. flake8 will emit the non-suppressed errors

So the solution is to only skip unselected checkers if flake8-noqa is also not in use.

[jakkdl]

Okay I finally got to my computer and actually tried this and as you say we get NQA102 when doing #noqa ... except that doesn't change when I stopped excluding visitors and it's also the case on main ... and also the case since the very beginning.

Once I breakpointed in flake8-noqa it was easy to figure out the problem though: we've been formatting our errors as TRIOxxx: ... and flake8-noqa splits that out as the error code being TRIOxxx: instead of TRIOxxx.

Once I got that working, I could check and you're very much right. Though this is only really a problem if you have noqas for ignored checks... which I don't really see why you should have? If it's a temporary thing, then you can easily also ignore NQA1xx. If it's a long-term thing then you shouldn't be noqaing those. If you're doing CLI debugging and only looking at specific errors with --select then you're probably not selecting NQA.

I can totally see a use case where a programmer wants to skip unselected visitors and also run flake8-noqa, so blanket disabling the skipping if NQA1xx is selected doesn't seem great to me.

I could add a configuration flag that turns off the feature, but I think the neatest would be if flake8-noqa has an option to disable NQA1xx for codes that aren't selected (which I think plinss/flake8-noqa#12 is for). Especially if PyCQA/flake8#751 gets implemented, and apparently pycodestyle already disables visitors that aren't selected.

[jakkdl]

After being dismissed by the flake8 maintainers (so it's probably never getting any similar functionality upstream and who knows when they'll break it) and sleeping on it I guess it's better to just split this off into a separate flag --enable-visitor-regex, defaulting to .*. This way you can:

1. ignore it, and just use the normal built-in ways of enabling/disabling checks. Guarantees flake8 compatibility but won't save any performance etc.
2. Not use the normal enable/disable checks for TRIO codes at all, and exclusively enable/disable codes with this.
3. Selectively disable broken or time-consuming checks that you're not gonna use.
4. Use both in a haphazard manner and bring doom and confusion upon yourself. Making it a regex parameter should scare off basic users, and will properly document that this is a bad idea.

That should simplify the code for this significantly, and get rid of any reliance on upstream behaving more than minimally required.

[jakkdl]

Commited changes as a separate commit to ease reviewing, but should probably be squashed upon merging.

[jakkdl]

rebased on top of main

[jakkdl]

Also, don't bother merging both this and #94, merge one and I'll fix the conflicts in the other.

[Zac-HD]

Honestly I'm still a bit dubious about exposing the regex argument to end-users, but it's expedient to do so and we can always take it out later I guess!

Otherwise looks great, thanks!

[jakkdl]

hehe I might've gotten overly comfortable with regex's as of late. But we could even opt to make this a fully undocumented flag or something, as it shouldn't really be used by the average user.