Skip to content

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

Merged
Zac-HD merged 3 commits into
python-trio:mainfrom
jakkdl:58_async_alternatives_to_blocking_io_calls
Dec 12, 2022
Merged

Zac-HD merged 3 commits into
python-trio:mainfrom
jakkdl:58_async_alternatives_to_blocking_io_calls

Conversation

@jakkdl

@jakkdl jakkdl commented Dec 8, 2022 •

Copy link
Copy Markdown
Member

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]

  • Currently it works with wildcards, only needed a minor modification to fnmatch_qualified_name for that to work smoothly, which doesn't affect the decorator matching using it.
  • Visitor200 is pretty simple
  • Couldn't find any built-in argparse dict parser, so wrote one myself. I would maybe have used : as the splitter if it was me, but easily configurable in ParseDict.
    • Considered splitting into two separate argumenterrors for no splitter tokens and >1 tokens, but idk if users need that much hand-holding
  • after you taught me about f{var!r} I couldn't resist adding it to ListOfIdentifiers as well ^^;

[pyproject.toml]

  • got tired of managing pyright complaining about typing errors in my test file, so added an exclude filter (and needed to explicitly add the default excludes). This means that a bunch of other test files could be cleaned up
    • Was there any reason this wasn't done from the start?

[tests/test_decorator.py]

  • test_decorator started breaking with another command-line option, so modified that to use the same construction as the test_xxx_options in test_flake8_trio.py

[tests/test_flake8_trio.py]

  • with the increasing number of warnings with configuration flags, and realizing we're back to manually specifying error codes and lines and shit, I added support for an # ARGS construct in a test file to specify command line flags to be used when running the file.
    • No obvious way to check malformed flags with that approach, so that's still in a separate test function
    • But test_113_options and test_114_options could be cleaned up given this new functionality. Leaving that for a later PR though.
  • I wanted capsys functionality in test_200_options, partly because testing is good:tm:, but in large part because I wanted to silence them in my CLI even when running with -s. So added that to test_114_options as well.

TODO:

  • CHANGELOG entry ✔️
  • README entry ✔️
  • README configuration flag documentation ✔️
  • The test file could maybe have some comments ✔️

@jakkdl jakkdl linked an issue Dec 8, 2022 that may be closed by this pull request
10 of 12 tasks
@jakkdl
jakkdl force-pushed the 58_async_alternatives_to_blocking_io_calls branch 2 times, most recently from 407849e to 89e52f0 Compare December 8, 2022 12:59
@jakkdl

jakkdl commented Dec 8, 2022

Copy link
Copy Markdown
Member Author

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.

Comment thread tests/trio200.py Outdated
@@ -0,0 +1,22 @@
# ARGS --trio200-blocking-calls=bar->BAR,bee->NOERROR,bonnet->NOERROR,bee.bonnet->BEEBONNET

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

is NOERROR special?

Copy link
Copy Markdown
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

nope, suppose I might as well change them to BEE and BONNET to avoid confusion

Copy link
Copy Markdown
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

or actually no that could in theory cause some other confusion/testing error since BEE and BONNET are substrings of BEEBONNET.
Maybe all magic words should be surrounded by underscores or something, not sure.

@Zac-HD

Zac-HD commented Dec 8, 2022

Copy link
Copy Markdown
Member

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

jakkdl commented Dec 9, 2022 •

Copy link
Copy Markdown
Member Author

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

jakkdl commented Dec 9, 2022 •

Copy link
Copy Markdown
Member Author

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
jakkdl force-pushed the 58_async_alternatives_to_blocking_io_calls branch from 9603138 to 8cd321a Compare December 9, 2022 11:09
@jakkdl

jakkdl commented Dec 9, 2022

Copy link
Copy Markdown
Member Author

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
jakkdl force-pushed the 58_async_alternatives_to_blocking_io_calls branch from 8cd321a to fe0ae2b Compare December 9, 2022 11:16
@jakkdl
jakkdl force-pushed the 58_async_alternatives_to_blocking_io_calls branch from fe0ae2b to e37c467 Compare December 9, 2022 11:19

@Zac-HD Zac-HD left a comment

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Looking good!

Comment thread README.md Resolved
Comment thread flake8_trio.py Resolved
Comment thread flake8_trio.py Resolved
Comment thread flake8_trio.py Resolved
Comment thread flake8_trio.py Resolved
@jakkdl
jakkdl force-pushed the 58_async_alternatives_to_blocking_io_calls branch from 610f995 to fea41fe Compare December 12, 2022 14:04
@jakkdl

jakkdl commented Dec 12, 2022

Copy link
Copy Markdown
Member Author

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.

@Zac-HD Zac-HD left a comment

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Looks great despite growing somewhat in the writing - thanks @jakkdl!

@Zac-HD
Zac-HD merged commit 483447b into python-trio:main Dec 12, 2022
@jakkdl
jakkdl deleted the 58_async_alternatives_to_blocking_io_calls branch December 31, 2022 13:06
@jakkdl jakkdl mentioned this pull request Jan 13, 2023
10 of 12 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

Successfully merging this pull request may close these issues.

Recommend async alternatives to blocking IO calls

2 participants