Refactor SSE client with timeout support and listen_until_clear() #13
Conversation
- changed listen method to a generic generator the yields the status sent by the server stream. - implemented listen_until_clear that simulates what block() does but for server streams insteam of polling.
|
When I try to run these changes I get: Obviously I can fetch the module myself, but there must be a way to add the package to some manifest so python knows to fetch it automatically. |
luketpeterson
left a comment
luketpeterson
left a comment
There was a problem hiding this comment.
I raised mainly style stuff. I think Adam had opinions on more substantive things in the API design.
|
|
||
| def on_error(): | ||
| raise Exception("error") | ||
| print("event stream closed") |
There was a problem hiding this comment.
I don't think we want to be this chatty, by default. It's handy for debugging, and maybe if we support a "verbose" switch. But otherwise we probably want operations at this level to be silent on the client side. Just my opinion.
python/client.py
Outdated
|
|
||
| status = msg.get("status") | ||
| if status == "pathClear": | ||
| print("path cleared") |
There was a problem hiding this comment.
See above about status chatter
python/client.py
Outdated
| print("path cleared") | ||
| return "" | ||
| elif status == "pathForbiddenTemporary": | ||
| print("waiting for path to clear") |
There was a problem hiding this comment.
See above about status chatter
python/client.py
Outdated
| # _main() | ||
| _main_mm2() | ||
| # test_sse_status() | ||
| # _main_mm2() |
There was a problem hiding this comment.
Let's restore the functionality of the client.py main to running MM2 tests.
Perhaps we want to move all tests outside of client.py, so client.py is just a "library" module, and the tests are in another file. That would be what I would do, but I don't come from the Python world so I don't know all the Python standard practices.
python/client.py
Outdated
| _main_mm2() | ||
| # test_sse_status() | ||
| # _main_mm2() | ||
| test_sse_status() |
There was a problem hiding this comment.
Even if we keep this test in this file, it probably ought to start with '_' so it doesn't look like a public API.
There was a problem hiding this comment.
i will resolve the above comments
I have added a requirements.txt file for the required dependencies. you can start a virtual environment for a separate python environment. python3 -m venv venv
source venv/bin/activate
pip install -r requirements.txt |
2 participants
listen()listen_until_clear(timeout)pathClearstatus is received or timeout is reached.block()..listen()calls with.listen_until_clear()