-
-
Notifications
You must be signed in to change notification settings - Fork 2
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
21X and 22X, blocking sync calls in async function #94
21X and 22X, blocking sync calls in async function #94
Conversation
e742720
to
9a5b9ac
Compare
9a5b9ac
to
14991c7
Compare
Not gonna bother composing beautiful error messages and readme texts until the split has been decided on. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Looks good!
14991c7
to
23eaaf3
Compare
23eaaf3
to
73e27fb
Compare
addressed comments, rebased on top of main. This PR is turning out quite meaty indeed. The documentation and error messages are still mediocre, feel free to suggest changes. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I think we need to walk back one heuristic (see below); otherwise looks great - I'm happy with these messages, so if you add the version bump we'll auto-release on merge!
flake8_trio.py
Outdated
"httpx" in self.imports | ||
and isinstance(node.func, ast.Attribute) | ||
and node.func.attr in http_methods | {"request", "stream", "send"} |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I think this is probably too false-alarm-prone; without the extra evidence from the first argument being a known constant we'll error on any method with one of these extremely common names. For example:
import httpx
some_dict.get(...) # should not raise a TRIO warning!
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
It still has to be inside an async function, and not be awaited. But yeah could also track the variable name a call to httpx.Client()
is saved to (and do the same with urllib3), as is currently done with nurseries.
Removed it for now, and bumped version.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
It still has to be inside an async function, and not be awaited. But yeah could also track the variable name a call to httpx.Client() is saved to (and do the same with urllib3), as is currently done with nurseries.
Probably a good follow-up PR once we get the other parts of #58 done 👍
…etwork and subprocess calls, respectively.
73e27fb
to
0ee8303
Compare
Checking off the first couple sync calls in #58
Flake8TrioVisitor.__subclasses__
didn't do it anymore once I started using multilevel inheritance, so solved it by creating a decorator for error classes that registers them. (type hinting the decorator function was fun 😅)Wasn't sure if the http calls and process invocation bullet points wanted different error messages for each, and the messages themselves are kinda rough, but went with this for a first pass.
TODO:
trio210.py
- change regex so it can be namedtrio2xx.py
or split intotrio21x.py
andtrio22x.py
Don't bother merging both this and #91, merge one and I'll fix the conflicts in the other.