Conversation
8b00ffb to
288e2d1
Compare
288e2d1 to
ec32fec
Compare
| offset = e.colno - 1 | ||
| first = json.loads(line[0:offset]) | ||
| if "hijack" not in first: | ||
| logging.warning("Working around docker 'hijack' message") |
There was a problem hiding this comment.
The error handling for the Docker 'hijack' message appears inconsistent. In the read_from_stdin() function, when a JSON decode error occurs, the code logs a warning with "Working around docker 'hijack' message" but doesn't verify that 'hijack' is actually present before proceeding. This contrasts with line 81 which uses assert "hijack" in first.
Consider either:
- Adding proper validation before the warning message:
if "hijack" in first: logging.warning(...) - Using a try/except block to handle the case where 'hijack' isn't present
- Removing the warning and letting the assertion handle the error case
This would make the error handling more robust and consistent.
| logging.warning("Working around docker 'hijack' message") | |
| if "hijack" in first: | |
| logging.warning("Working around docker 'hijack' message") |
Spotted by Diamond
Is this helpful? React 👍 or 👎 to let us know.
| version = "1.0.0" | ||
| description = "River toolkit for Python" | ||
| readme = "README.md" | ||
| requires-python = ">=3.12" |
There was a problem hiding this comment.
There's a version mismatch between the pyproject.toml file which requires Python ≥3.12 and the client.dockerfile which uses Python 3.11. This could lead to runtime issues or dependency conflicts. Consider either:
-
Updating the dockerfile to use Python 3.12+:
FROM python:3.12-slim-bookworm -
Or adjusting the
pyproject.tomlrequirements to match the Docker environment:requires-python = ">=3.11"
Ensuring version consistency will help prevent potential deployment problems.
| requires-python = ">=3.12" | |
| +requires-python = ">=3.11" |
Spotted by Diamond
Is this helpful? React 👍 or 👎 to let us know.
No description provided.