Open oriordan opened 5 months ago
Hey @oriordan, thank you for your contributions.
This is a significant change, and as I haven't written a contribution guide, I'll leave it open for code review and add comments sporadically when time permits. As I'm currently addressing some issues in the code, anticipate potential breaking changes and rebase as necessary.
In the future, I would prefer PRs that focus on smaller, single-issue changes. I would also appreciate an issue request being opened beforehand to discuss any enhancements or additions to the codebase.
Please don't be discouraged if you wish to contribute further. For substantial changes, it's advisable to open a draft PR and incrementally discuss the changes.
Thank you for your time. I really appreciate it.
To focus on small PR changes and on specific context, may you open a new PR where you can integrate the logging behavior only into the codebase? Is this OK for you?
Sure, can do that tonight. So just to agree:
debug_method_map
completely: I don't think stable code needs thisIf you want to keep something similar to debug_method_map
, I suggest to create a new logger for every method and send logs via that. This allows to have a very granular logging control over the different methods. Example:
method_logger = logging.getLogger(f"{__name__}.{method}")
method_logger.debug("Request: %s %s", method, params)
method_logger.debug("Response: %s", response)
Then if someone wants to only debug certain methods, they can just enable/disable the required loggers only.
wdyt?
The debug_method_map
was implemented to disable stream messages when sending a chat request. I think in the future we may implement a stream chat function to handle this correctly.
Let us have the logger as a Singleton, where the user can toggle the debugging. For now, the logging in the chat
function need to be disabled for the webpost_request
(sorry, forget the name, but can be inspected in the function).
To implement the logger functionality, we can put it in a logger.py
file.
I appreciate your work.
All right, this is a big one, apologies for that. Have been working on this for a few days and rebased it on your most recent changes. Happy to discuss anything, it's most likely breaking some of your existing stuff, but in general I think this brings the code to a shape which makes it much easier to use.
What I have not done but would seriously consider:
Most code is still sync out there and testing/prototyping async code is more pain. Since you went native async from day one, there is no need to make a sync copy but just wrapping around all methods in a sync equivalent works pretty well.