Closed jakkdl closed 1 year ago
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 noqa
d lines at a later stage. But I haven't tried it
Let's suppose that I have some file that has a # noqa
'd trio200 error. If I then run flake8 --select=TRIO102,NQA
:
So the solution is to only skip unselected checkers if flake8-noqa is also not in use.
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 noqa
s 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 noqa
ing 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 https://github.com/plinss/flake8-noqa/issues/12 is for). Especially if https://github.com/PyCQA/flake8/issues/751 gets implemented, and apparently pycodestyle already disables visitors that aren't selected.
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:
That should simplify the code for this significantly, and get rid of any reliance on upstream behaving more than minimally required.
Commited changes as a separate commit to ease reviewing, but should probably be squashed upon merging.
rebased on top of main
Also, don't bother merging both this and #94, merge one and I'll fix the conflicts in the other.
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.
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, andtest_noerror_on_sync_code
is now way cleaner when it can just specify--ignore
for the ignored error codes. But needed some restructuring to callinitialize_options
in several places.