-
Notifications
You must be signed in to change notification settings - Fork 32
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
Improve Python documentation #217
Conversation
7c3e0a6
to
2d32dd5
Compare
|
||
def enable_log_forwarding(level: str) -> None: | ||
""" | ||
Enable SDK logging to stdio. |
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.
Is it stdio, necessarily? We're forwarding logs to the logging
facility. Since that's more or less what a python user would expect, perhaps we simply call this enable_logging(level:str)
and document it as such.
Out of curiosity... how does pyo3-log populate the name
, pathname
, and lineno
in the LogRecord
s it generates?
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.
Good point. I'll update the description. Does enable_logging
seem clearer than enable_log_forwarding
? Might it be confused with log()
ging? This is the internal module, so being verbose & explicit seems OK to me.
how does pyo3-log populate...
Good question; I'm not too familiar with it. ...Looks like it just uses information from the Rust log record directly (with some substitution in the name): https://github.com/vorner/pyo3-log/blob/835647f0baf76e0af28178f8a3a63df25849fdfb/src/lib.rs#L439-L440
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.
Good point. I'll update the description. Does
enable_logging
seem clearer thanenable_log_forwarding
? Might it be confused withlog()
ging? This is the internal module, so being verbose & explicit seems OK to me.
Right, my bad, I was referring to the actual #[pyfunction]
, which currently shares the same name. I think we should take "forwarding" out of the name, because from a user's perspective this is just a python library. They don't care about the fact that our extension is written in rust, or that some form of "forwarding" is required to port log messages across the boundary.
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.
This is in the _foxglove_py
module, which end users shouldn't be importing from. I think it should still be documented, but I think it's OK to make this more aware of the internals. But for now I updated the names; easy to update later.
But I agree with you on the public API. The methods there are currently named verbose_on
and verbose_off
. (I'm not a fan of those names, but I think they were chosen for the reason above — to avoid confusion with log()
). The docstrings there now say "enable/disable logging".
|
||
def get_channel_for_topic(topic: str) -> BaseChannel: | ||
""" | ||
Get a channel for the given topic. |
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.
"previously-registered channel"?
This PR improves the documentation of the Python SDK.
Runtime improvements:
pyclass
es, which sets__module__
<class 'foxglove.schemas.Color'>
instead of<class 'builtins.Color'>
)__repr__
for schema and channel instances (e.g.SceneUpdateChannel(topic='/boxes')
instead of<foxglove.channels.SceneUpdaterChannel object at 0x103717190>
)Note
To support channel printing, this adds a public
topic()
getter onTypedChannel
in core.Other changes:
examples/ws-demo.py
toexamples/live_visualization.py