python: support status publishing on server #227
Conversation
bryfox
force-pushed
the
bryan/fg-10188-python-support-send_status-remove_status
branch
from
cb458d2 to
bd6b7a0
Compare
| Exercise the server interface; will also be checked with mypy. | ||
| """ | ||
| server = start_server() | ||
| server.publish_status(Status(StatusLevel.Info, "test", "some-id")) |
There was a problem hiding this comment.
Maybe we can just make these function arguments, instead of using a class like rust does. Saves an allocation, which doesn't matter much here. But it also is one less type for the user to care about.
There was a problem hiding this comment.
Strongly agree. I had similar feedback on the rust PR about making this a bit more ergonomic. Maybe:
def publish_status(level: StatusLevel, message: str, id: str | None = None):| } | ||
|
|
||
| /// Publishes the current server timestamp to all clients. | ||
| pub fn broadcast_time(&self, timestamp_nanos: u64) { |
There was a problem hiding this comment.
Is this from another PR that you used as a base? Not a big deal, I'll review that one next. You can figure out the logistics around merging them.
There was a problem hiding this comment.
Sorry, this is a wonky diff because I changed the conditionals to early returns; was added in #224.
| return; | ||
| }; | ||
| server.remove_status(status_ids); | ||
| } |
There was a problem hiding this comment.
A bigger question about the API. Is it better to silently fail after the server is closed, or to throw an exception?
Since the default is to only close the server at exit, this seems ok to me as-is.
There was a problem hiding this comment.
Either way, we should probably document that behavior.
It would be user-friendlier to not raise an exception, so that users aren't forced to litter their code with with contextlib.suppress(...): statements, but my rusty side says it's better to explicitly raise errors instead of trying to guess what the user was trying to do. Since we're trying to gear our python SDK towards user-friendliness, I suppose I agree with Dan.
python/foxglove-sdk/src/lib.rs
Outdated
| m.add_class::<PyCapability>()?; | ||
| m.add_class::<PyClient>()?; | ||
| m.add_class::<PyClientChannelView>()?; | ||
| m.add_class::<PyStatus>()?; | ||
| m.add_class::<PyStatusLevel>()?; |
There was a problem hiding this comment.
Since these have fairly generic names ("capability", "client", "status", etc.), we chose to nest them under the websocket module in rust. Should we do the same in python?
There was a problem hiding this comment.
Yeah, I'd certainly like to re-organize this. Since there are a few concurrent PRs in-flight, I'd like to take a pass at that in a follow-up PR. I think it's mildly unfortunate we're baking the name "websocket" into these interfaces, but I'll likely follow whatever rust does in the end.
| let Some(server) = &self.0 else { | ||
| return; | ||
| }; | ||
| server.clear_session(session_id); |
There was a problem hiding this comment.
Ultra-nit: you could write this a bit more succinctly:
| let Some(server) = &self.0 else { | |
| return; | |
| }; | |
| server.clear_session(session_id); | |
| if let Some(server) = &self.0 { | |
| server.clear_session(session_id); | |
| } |
| return; | ||
| }; | ||
| server.remove_status(status_ids); | ||
| } |
There was a problem hiding this comment.
Either way, we should probably document that behavior.
It would be user-friendlier to not raise an exception, so that users aren't forced to litter their code with with contextlib.suppress(...): statements, but my rusty side says it's better to explicitly raise errors instead of trying to guess what the user was trying to do. Since we're trying to gear our python SDK towards user-friendliness, I suppose I agree with Dan.
| Exercise the server interface; will also be checked with mypy. | ||
| """ | ||
| server = start_server() | ||
| server.publish_status(Status(StatusLevel.Info, "test", "some-id")) |
There was a problem hiding this comment.
Strongly agree. I had similar feedback on the rust PR about making this a bit more ergonomic. Maybe:
def publish_status(level: StatusLevel, message: str, id: str | None = None):
bryfox
deleted the
bryan/fg-10188-python-support-send_status-remove_status
branch
This adds
publish_statusandremove_statusto the Python server interface, to match the Rust SDK.I've also updated the existing server methods to not raise exceptions, since the Rust SDK doesn't return resuts. Currently, the only way for these to fail is if the server has been shut down.
This is based on #226 to minimize conflicts (because that re-organized the structure of the server-related code).