Conversation
… A3ValidationError
![gemini-code-assist[bot]](https://avatars.githubusercontent.com/in/956858?s=60&v=4){kind=small}
There was a problem hiding this comment.
Code Review
This pull request enhances the Python implementation of the Amino Acid Annotation (A3) format by adding license information, updating project metadata, and refining validation logic. Key improvements include the explicit rejection of boolean values in position fields and more robust error handling that collects multiple validation issues before raising an exception. Tests were updated to reflect these changes, and several dependencies were bumped. Feedback was provided regarding a redundant boolean check in the range normalization logic that is already covered by the Position type validator.
| if isinstance(s, bool) or isinstance(e, bool): | ||
| raise ValueError("boolean values are not valid positions") |
There was a problem hiding this comment.
The check for boolean values here is redundant. The RegionEntry model defines the index field as list[tuple[Position, Position]], and the Position type alias already includes a BeforeValidator(_reject_bool) that handles this check. Since _normalize_ranges is a before validator, Pydantic will subsequently validate its output against the field's type annotation, making this explicit check unnecessary. Relying on the Position type for this validation will make the code more concise and less repetitive.
There was a problem hiding this comment.
Pull request overview
Updates the Python A3 implementation to tighten validation semantics (especially around booleans and bounds checking), improve error reporting ergonomics, and polish package/repo metadata.
Changes:
- Add explicit rejection of boolean values where integers are expected (positions/ranges) and improve bounds-check error aggregation.
- Adjust JSON envelope handling (
$schema,a3_version) and update tests accordingly. - Refresh package metadata (license, classifiers/URLs) and update docs/badges/lockfile.
Reviewed changes
Copilot reviewed 9 out of 10 changed files in this pull request and generated 3 comments.
Show a summary per file
| File | Description |
|---|---|
| README.md | Adds a Crates.io version badge for the Rust crate. |
| python/rtemis_a3/uv.lock | Updates locked tool/dependency versions (e.g., click/ruff/ty/zensical). |
| python/rtemis_a3/tests/test_models.py | Adds boolean-rejection tests; switches bounds tests to the functional API and asserts multi-error behavior. |
| python/rtemis_a3/tests/test_api.py | Updates envelope expectations (defaults + schema/version validation) and adds wrong-schema/version tests. |
| python/rtemis_a3/src/rtemis/a3/errors.py | Expands A3ValidationError docs and adds a messages convenience property. |
| python/rtemis_a3/src/rtemis/a3/api.py | Adds envelope validation and wraps model bounds failures into A3ValidationError. |
| python/rtemis_a3/src/rtemis/a3/_models.py | Introduces _BoundsErrors, adds boolean rejection for positions/ranges, and changes bounds failures to raise _BoundsErrors. |
| python/rtemis_a3/README.md | Updates the wire-format example to include $schema and a3_version. |
| python/rtemis_a3/pyproject.toml | Improves project metadata (description, license, classifiers, URLs). |
| python/rtemis_a3/LICENSE | Adds MPL-2.0 license text to the Python package. |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
| except _BoundsErrors as exc: | ||
| errors = [{"loc": (), "msg": msg, "type": "value_error"} for msg in exc.messages] | ||
| raise A3ValidationError("\n".join(exc.messages), errors) from exc |
There was a problem hiding this comment.
_BoundsErrors are converted into A3ValidationError.errors, but the generated entries all use loc=() and omit the offending field path. This makes the structured errors list much less useful for programmatic consumers and also conflicts with the docstring claim that loc contains field-path components. Consider having _BoundsErrors carry structured error objects (with loc tuples), or parse the path prefix in each message into a loc tuple when building errors.
| except _BoundsErrors as exc: | ||
| errors = [{"loc": (), "msg": msg, "type": "value_error"} for msg in exc.messages] | ||
| raise A3ValidationError("\n".join(exc.messages), errors) from exc |
There was a problem hiding this comment.
Same issue as above: bounds-check violations are mapped to errors entries with loc=(), losing the field path and limiting how callers can present/associate errors. Prefer emitting structured loc tuples for each bounds violation (either by carrying them in _BoundsErrors or deriving them here).
| Structured error list. Each entry has at minimum ``loc`` (tuple of | ||
| field path components), ``msg`` (human-readable message), and | ||
| ``type`` (error kind string). Pydantic structural errors follow | ||
| Pydantic's native format; bounds and envelope errors follow the same | ||
| shape for consistency. |
There was a problem hiding this comment.
The docstring says bounds errors “follow the same shape” as Pydantic errors with loc as a tuple of field-path components, but bounds violations are currently surfaced with loc=() in api.py. Either adjust the documentation here to reflect the actual structure, or (preferably) emit per-violation loc tuples for bounds errors to match the documented contract.
| Structured error list. Each entry has at minimum ``loc`` (tuple of | |
| field path components), ``msg`` (human-readable message), and | |
| ``type`` (error kind string). Pydantic structural errors follow | |
| Pydantic's native format; bounds and envelope errors follow the same | |
| shape for consistency. | |
| Structured error list. Each entry has at minimum ``loc`` (a tuple of | |
| field path components, or ``()`` for root-level/non-field-specific | |
| violations), ``msg`` (human-readable message), and ``type`` (error | |
| kind string). Pydantic structural errors follow Pydantic's native | |
| format; contextual errors such as bounds or envelope violations may | |
| use an empty ``loc`` when no specific field path applies. |
No description provided.