Conversation
Additionally fixes an issue where `ValidationError(error)` constructor would re-raise an exception because it requires additional arguments.
|
Looks good, thanks. |
That is nice, I do think it's a good fit here, feel free to change it back in the PR. Now, if only we could just remove exceptions from the language and call it a day! |
There was a problem hiding this comment.
Pull request overview
This PR improves SMP frame parsing diagnostics by introducing a dedicated validation exception that includes detailed context, and updates the lint alias to show Black diffs.
Changes:
- Add
SMPValidationExceptionto carry a short message plus detailed parsing diagnostics. - Enhance
SMPClient.request()to collect Pydantic validation errors across candidate response/error types and log a hexdump + error breakdown before raisingSMPValidationException. - Update the
lintalias to runblack --check --diff.
Reviewed changes
Copilot reviewed 3 out of 3 changed files in this pull request and generated 4 comments.
| File | Description |
|---|---|
| smpclient/exceptions.py | Adds a new exception type for response validation failures. |
| smpclient/init.py | Implements detailed validation error formatting/logging and raises SMPValidationException on parse failure. |
| envr-default | Updates the lint alias to include Black --diff output. |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
|
|
||
|
|
||
| def _hexdump_ascii(data: bytes) -> str: | ||
| """Python 3.12+ has builtin hexdump, prior to that we need to reinvent the wheel.""" |
There was a problem hiding this comment.
The _hexdump_ascii docstring claims Python 3.12+ has a built-in hexdump; the stdlib doesn't provide a general-purpose bytes hexdump helper. Consider updating the docstring to reflect why this helper exists (e.g., for readable debug logging) without referencing a non-existent built-in.
| """Python 3.12+ has builtin hexdump, prior to that we need to reinvent the wheel.""" | |
| """Return a human-readable hex + ASCII dump of the given bytes for debug logging.""" |
| msg, details = _smp_validation_error_message(header, frame, errs) | ||
| logger.error(msg + details) | ||
| raise SMPValidationException(msg, details) |
There was a problem hiding this comment.
request() now raises SMPValidationException instead of bubbling a pydantic.ValidationError when the frame can't be parsed. There are existing SMPClient request tests, but none covering this new failure mode; please add/adjust a test to assert the new exception type and (at minimum) that details contains header/frame context.
| err_type = err["type"] | ||
| msg = err["msg"] | ||
| loc = ".".join(str(x) for x in err["loc"]) | ||
| lines.append(f"\t\t[{err_type}] {msg}: {loc}; input: {err['input']})") |
There was a problem hiding this comment.
The formatted error line ends with an unmatched closing parenthesis ("... input: ...)"). This will produce confusing log output; remove the trailing ")" or add a corresponding opening delimiter if you intended to wrap the input value.
| lines.append(f"\t\t[{err_type}] {msg}: {loc}; input: {err['input']})") | |
| lines.append(f"\t\t[{err_type}] {msg}: {loc}; input: {err['input']}") |
| def _smp_validation_error_message( | ||
| header: smpheader.Header, | ||
| frame: bytes, | ||
| errs: dict[type[smpmsg.Response], ValidationError], |
There was a problem hiding this comment.
Type annotations for errs are inconsistent: _smp_validation_error_message() declares errs: dict[type[smpmsg.Response], ValidationError], but request() builds errs: dict[Type, ValidationError] and includes error classes (_ErrorV1/_ErrorV2). This is likely to fail mypy due to dict invariance. Align the types (e.g., make both use dict[type[object] | type[Any], ValidationError], or tighten request() to the exact common base type).
| errs: dict[type[smpmsg.Response], ValidationError], | |
| errs: dict[Type, ValidationError], |
2 participants
No description provided.