-
Notifications
You must be signed in to change notification settings - Fork 79
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
release lock only if acquired #249
Conversation
Codecov ReportPatch coverage:
Additional details and impacted files@@ Coverage Diff @@
## main #249 +/- ##
=====================================
Coverage 84.7% 84.7%
=====================================
Files 4 4
Lines 472 473 +1
Branches 87 88 +1
=====================================
+ Hits 400 401 +1
Misses 45 45
Partials 27 27
☔ View full report in Codecov by Sentry. |
Good catch, and thank you for the PR with a fix! 😊 I wasn't sure if we should allow calling I tested Could you add a test case for this? (Probably sufficient to test if we can call |
Like this?
Imho it only makes sense, there is nothing to gain if it raises an error. |
Using a context manager is the only recommended method and the main direction of subsequent maintenance, because the removal of the connect and disconnect methods is already planned #247.
This does not happen if you use a context manager. Based on PEP343, this PR is great. |
I'm aware of that and the docs make this clear. |
For your use case, using The test looks good. Thank you for your contribution to aiomqtt! 😎 |
Follow up empicano#249
If connection fails calling
__aexit__
will result in an Lock-exception.This small check prevents that