python-trio / flake8-async

Highly opinionated linter for Trio code
https://flake8-async.readthedocs.io
MIT License
17 stars 2 forks source link

Add TRIO200, user-configured warnings for blocking sync calls in async function #80

Closed jakkdl closed 1 year ago

jakkdl commented 1 year ago

Implements the config option in #58 This ended up touching surprisingly many spots in various places, most of them unintentionally. So sorry for the diff being a bit sprawling. Going through the diff and explaining all changes:

[flake8_trio.py]

[pyproject.toml]

[tests/test_decorator.py]

[tests/test_flake8_trio.py]

TODO:

jakkdl commented 1 year ago

sad, I wrote and confirmed that a version with ast.unparse, removing construct_name completely, works perfectly with current tests - until 3.8 came and spoiled all the fun.

Zac-HD commented 1 year ago

Looks good so far! How much would break if we just declared that 3.8 was only best-effort support? I don't really mind requiring 3.9+ to run the linter, now that 3.11 is out.

My (weakish) preference for -> over : is that the latter often has special meaning in config file formats, or can be confused with attribute/member lookup from other languages. -> to me reads more directly as "replace with" but I'm open to arguments against!

jakkdl commented 1 year ago

For just this change it would break 107 and 108 with a user-configured decorator, 200, and almost certainly all other 2xx messages. But there's generally some developing overhead to account for 3.8, since the AST has changed since then (e.g. https://github.com/PyCQA/flake8-bugbear/blob/main/bugbear.py#L683-L689) so it can sometimes be a non-insignificant overhead to debug issues caused by it, and even if that's just a matter of skipping broken tests you'd still have to figure out where/why/how it breaks and insert skips, and also update tests to skip it. So I personally wouldn't complain if we stopped testing on it entirely (and running the full test suite would be faster, yay!).

jakkdl commented 1 year ago

My (weakish) preference for -> over : is that the latter often has special meaning in config file formats, or can be confused with attribute/member lookup from other languages. -> to me reads more directly as "replace with" but I'm open to arguments against!

Makes sense. I'm mostly just colored by mentally parsing it as a dict, and seeing -> just looks a bit weird. But since we don't require {} so it's not a proper dict it's maybe a bad idea to reinforce the idea that it's parsed into one under the hood.

jakkdl commented 1 year ago

fixed todo's, changed fnmatch_qualified_name and started messing with disabling tests for 3.8 - but now it fails because of test coverage, so would need to configure --no-cov only when checking py38...

jakkdl commented 1 year ago

Fixed all comments, needed to split a couple lines that were long - and realized I've been inconsistent whether spaces should be at the start of the new line or at the end of the previous. Didn't find any great source on what's the preferred style, but seems like trailing space is more common. (Something for Black?)

shed also went ahead with a bunch of changes (though removing Union from trio107 it didn't do until I experimented with --py311-plus despite it working perfectly fine with py39).

I also managed to get rid of all pragma: no cover in the whole codebase (:tada:), partly thanks to pyright no longer checking the test files so I can do highly illegal stuff. Ran a --runslow as well to check that I didn't mess anything up.