-
Notifications
You must be signed in to change notification settings - Fork 3
add context manager #6
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
Conversation
add a context manager for lighter syntax and guarantee connection closure
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 great, I'm totally onboard with the idea API-wise.
The class docstrings / readme should probably mention that they can be used as context managers. But I'm happy to merge this and then add that after this PR :)
I don't specify anywhere in the project so this is on me: this project uses black for code formatting so could you stick it through that (I should really have CI for this...)
PS You're right on the asserts: those methods aren't meant to be called outside (and so aren't documented), and inside all usages are safe so the assertion is mainly for type checking reasons
Edit: I just saw your comment about docstrings, for context manager dunders no docstring is the way to go (as they'll never be seen) so it's already complete w.r.t that
- __enter__ and __ aenter__ now rely on handle_socket_errors to make sure socket is alive - __enter__ and __aenter__ are now typed with the class name instead of Self to maintain compatibility with python 3.8+ - formatted the whole file with black
|
All requested changes done (if I haven't missed any). |
|
Hey are you still working on this? It'd be nice to land these changes :) |
|
i'm sorry i've been really busy lately, i work on it as soon as i can |
|
Don't worry there's no rush at all! Just wanted to see if you were still interested in doing this, so take a look at this whenever you want |
- typing now use string instead of __future__ annotations - synchronous context manager now ignore the connect_on_init parameter and connect the client by default - context managers will attempt to connect the client if not already connected - context managers will attempt to reconnect the client if in an error state
|
sorry for the long radio silence, this should be good now |
|
Looks great! |
proposal: add a context manager for lighter syntax and guarantee connection closure so instead of this:
we can simply do:
also for async clients:
I have to admit that this is my first pull request and I may be doing things wrong, I also haven't found a suitable docstring to these methods.
(also this probably need it's own ticket/pull request but you're using assert in prod in both
receive_exactlymethods instead of raising RCONNotConnected)