python-trio / flake8-async

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

Fix TRIO103 false alarm on excepts after a previous except has caught Cancelled #109

Closed jakkdl closed 1 year ago

jakkdl commented 1 year ago

Fixes #106

Also fixes several other cases not mentioned in the issue, and does the same for TRIO104.

Took me a minute to understand the existing 103/104 implementation and where to inject the logic for this, but ultimately a pretty simple addition.

It broke, like, all of the 103/104 tests though since they didn't bother having separate trys for the excepts, so went ham on adding those. But I didn't bother fixing trio103_no_104.py and cut that down to the minimal tests needed to check what that one actually checks. (that 103 is enabled, and not 104, when only one of them is selected with the regex). But at the end of trio103.py and trio104.py are the new test cases added

jakkdl commented 1 year ago

We should also make the README and the message for BaseException suggest this pattern.

If i try to get python highlighting in the code block in the readme, shed errors out

Could not parse ```python markdown block in file 'README.md'
    InvalidInput: Cannot parse: 1:0: except trio.Cancelled:(

since an except without a try is invalid syntax. Not sure if it's possible to tell shed (or whatever part of shed it is that errors out) to ignore the block.

Not sure how to format it the neatest inside the bullet list anyway.

Having different messages for an error hasn't actually been done before, and doing it this way is quite clunky. It feels like there should be some way of specifying the error message as like

"TRIO103": "{0} block with a code path that doesn't re-raise the error.{'more text here' if {0} != 'trio.Cancelled else ''}",

and get str.format to be as powerful as an f-string, but some very quick googling only turns up eval'ing it with an f added to the beginning of it. Not too necessary for this case, but if we do it more (and the 2xx's would probably have used this instead of having separate codes - which I didn't wanna do in this case) I'd like to get a better solution.

I also just realized that I've been horribly inconsistent in whether error messages end with a period or not.

jakkdl commented 1 year ago

With pass and raise looking somewhat similar when viewing the diff, I went ahead and replaced all the pass with ... in the test files so it's more clear where there's a raise.

jakkdl commented 1 year ago

Committed suggestions, fixed tests, and squashed.