Query context manager #461
Conversation
Only the `IntoPython` bound is important, the rest was there to reinforce the typing, but it's not needed at all, and it prevented to use objects not implementing `IntoRust` (which will be the case when `Query` will use `option_wrapper`)
|
PR missing one of the required labels: {'enhancement', 'bug', 'internal', 'documentation', 'breaking-change', 'new feature', 'dependencies'} |
wyfo
added
enhancement
zenoh/__init__.pyi
Outdated
| @@ -552,6 +554,7 @@ class Query: | |||
| By default, queries only accept replies whose key expression intersects with the query's. Unless the query has enabled disjoint replies (you can check this through Query::accepts_replies), replying on a disjoint key expression will result in an error when resolving the reply. | |||
| """ | |||
|
|
|||
| def drop(self): ... | |||
There was a problem hiding this comment.
I think drop requires a documentation, with a good explanation, why it is needed. I would also stress that user MUST call it once he provided all the replies (or use context) since I'm not sure we can entirely trust python to call the destructor.
There was a problem hiding this comment.
Yes it does, give me a few minutes.
There was a problem hiding this comment.
I would probably also mention in reply, reply_del and reply_err methods, that multiple replies are possible and that they are only sent once drop is called.
There was a problem hiding this comment.
I've added the docstring (I didn't have the necessary inspiration earlier).
The documentation of reply_xxx are aligned on the Rust API. Updating them is not the subject of this PR.
There was a problem hiding this comment.
Yes, but in rust drop happens automatically, and here we need to do it manually, so I would rather update it, to avoid false bug reports from users.
There was a problem hiding this comment.
What you said is not exact. The responses are sent directly, it's just the ResponseFinal message that is sent when the query is dropped. And because default consolidation is latest, get waits for the ResponseFinal to return the responses.
By the way, if you don't reply anything, but don't drop the query, get will still hang. So there is nothing about reply_xxx methods there.
There was a problem hiding this comment.
yes, there is nothing about reply_xxx to it, but these kind of methods are likely to be used by user, so he is more likely to see the docs and learn that it is mandatory to call drop, otherwise I do not see how to let user know that he needs to call drop if he is unaware of its existence.
Based on #460
Fixes #459