Logging Systems #15
Merged
Logging Systems #15
Conversation
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
- Remove empty line from index.md - Remove index.md from mkdocs.yml sections list
Rename function parameter from 'params' to 'arguments' across Lua logging system and test files to improve code consistency and clarity. Co-Authored-By: Claude <noreply@anthropic.com>
- Add logging import and logger initialization - Log connection attempts and successful connections - Log API request names and completion status - Log errors for connection failures and API errors - Log disconnection events Co-Authored-By: Claude <noreply@anthropic.com>
- Add logging-systems.md covering JSONL run logging, Python SDK logging, and mod logging - Document JSONL format specification for run replay functionality - Include Python SDK logging configuration examples - Add logging systems page to mkdocs navigation Co-Authored-By: Claude <noreply@anthropic.com>
Contributor
There was a problem hiding this comment.
Pull Request Overview
Adds a unified logging system across Lua hooks and the Python SDK, renaming the JSONL key from params to arguments and documenting the new behavior.
- Rename all occurrences of
paramstoargumentsin JSONL logs, Lua type annotations, log hook implementations, and tests. - Introduce Python SDK logging (using the
loggingmodule) inBalatroClientto log connections, API requests, and errors. - Add
docs/logging-systems.mdand updatemkdocs.ymlnavigation to document the three logging systems.
Reviewed Changes
Copilot reviewed 10 out of 10 changed files in this pull request and generated 2 comments.
Show a summary per file
| File | Description |
|---|---|
| tests/runs/one_ante_no_shop_2.jsonl | Updated JSONL entries to use "arguments" instead of "params". |
| tests/runs/one_ante_no_shop_1.jsonl | Same key rename for JSONL entries. |
| tests/lua/test_runs.py | Updated test logic to reference function_call["arguments"]. |
| src/lua/types.lua | Updated LuaDoc annotation for LogEntry to use arguments. |
| src/lua/log.lua | Renamed hook parameter variables and JSON field from params to arguments. |
| src/balatrobot/client.py | Imported logging, instantiated a logger, and added log statements around connect, request, and error handling. |
| mkdocs.yml | Added logging-systems.md to the site nav; removed an outdated index.md entry in the plugin section. |
| docs/logging-systems.md | New documentation page describing the three logging systems. |
| docs/index.md | Cleaned up stray markdown formatting. |
| .claude/settings.json | Locked down editing of CHANGELOG.md. |
Comments suppressed due to low confidence (3)
src/lua/types.lua:125
- Remove the outdated commented annotation for
paramssince the type now usesarguments, avoiding confusion in LuaDoc.
---@field timestamp_ms number Timestamp in milliseconds since epoch
mkdocs.yml:41
- The plugin's
Documentationsection removedindex.mdbut hasn't been updated to include the newlogging-systems.md; ensure the plugin configuration lists the new file so it's surfaced.
Documentation:
src/balatrobot/client.py:22
- No tests currently verify that the new logger emits the expected messages; consider adding unit tests (e.g., with pytest's
caplog) to exercise connection and API request logs.
logger = logging.getLogger(__name__)
src/balatrobot/client.py
Outdated
|
|
||
| # Create and validate request | ||
| request = APIRequest(name=name, arguments=arguments) | ||
| logger.info(f"Sending API request: {name}") |
There was a problem hiding this comment.
[nitpick] Consider lowering this to a debug-level log (logger.debug) for routine API messages so INFO-level remains focused on higher-priority events.
Suggested change
| logger.info(f"Sending API request: {name}") | |
| logger.debug(f"Sending API request: {name}") |
src/balatrobot/client.py
Outdated
| logger.error(f"API request {name} failed: {response_data.get('error')}") | ||
| raise create_exception_from_error_response(response_data) | ||
|
|
||
| logger.info(f"API request {name} completed successfully") |
There was a problem hiding this comment.
[nitpick] Similarly, you may want to use a debug-level log here to reduce noise in INFO-level logs during normal operation.
Suggested change
| logger.info(f"API request {name} completed successfully") | |
| logger.debug(f"API request {name} completed successfully") |
Changes log level from info to debug for API request start and completion messages to reduce noise in standard output while maintaining detailed debugging capability. Co-Authored-By: Claude <noreply@anthropic.com>
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
1 participant
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
No description provided.