-
-
Notifications
You must be signed in to change notification settings - Fork 3
async102 not applicable to asyncio #393
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
async102 not applicable to asyncio #393
Conversation
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.
Thanks, @RomanValov - this looks great!
If @jakkdl wants to confirm that'd be great; otherwise I'll merge in a few days 🚀
I'm not really sure sprinkling "ASYNCIO_NO_ERROR" across eval files is a way to go, in particular:
Any other comments would be welcome |
No this is perfect.
Ye this would ideally be fixed. I can do it in a followup if you're not comfortable, but you can copy tests from pretty much any other universal rule that does not have autofix. ASYNC109 looks like a good and simple candidate without any library-specific logic. |
if self._critical_scope.name == "except": | ||
self._potential_120.append((node, self._critical_scope)) | ||
else: | ||
# not applicable to asyncio due to different cancellation semantics it uses | ||
elif self.library != ("asyncio",): | ||
self.error(node, self._critical_scope, error_code="ASYNC102") |
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.
ASYNC120 has the same issue as ASYNC102 wrt asyncio, so if you don't mind you can expand the scope of this PR to also cover async120 with a small logic change here.
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.
as per my understanding async120 is still relevant to asyncio.
in case of async102 -- if cancellation occured in try block, it remains in except/finally blocks with trio/anyio
in case of async120 -- it only checks cancellation occurs during except ...
block which is still valid for asyncio
please correct me if I'm missing something
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.
sorry no, you're totally right. Ignore me
753278e
to
4146bb4
Compare
Hm. I'm pretty okay with
Done |
minor fixes
Everything looks perfect now, thanks for the PR!
I suppose the marker is somewhat confusingly named. Should maybe be something like |
Hey @RomanValov, it looks like that was the first time we merged one of your PRs! Thanks so much! 🎉 🎂 If you want to keep contributing, we'd love to have you. So, I just sent you an invitation to join the python-trio organization on Github! If you accept, then here's what will happen:
If you want to read more, here's the relevant section in our contributing guide. Alternatively, you're free to decline or ignore the invitation. You'll still be able to contribute as much or as little as you like, and I won't hassle you about joining again. But if you ever change your mind, just let us know and we'll send another invitation. We'd love to have you, but more importantly we want you to do whatever's best for you. If you have any questions, well... I am just a humble Python script, so I probably can't help. But please do post a comment here, or in our chat, or on our forum, whatever's easiest, and someone will help you out! |
Whilst #257 is not implemented do not trigger ASYNC102 for asyncio