Conversation
There was a problem hiding this comment.
Pull request overview
Aligns schema validation behavior across language implementations by rejecting duplicate positions (instead of silently deduplicating), addressing parse errors around duplicate indexes.
Changes:
- TypeScript: positions are now sorted but duplicate positions cause validation failure (with updated tests).
- Rust / Python / Julia: normalization/validation updated to reject duplicate positions, with corresponding test updates.
- Build/docs tooling: Makefile
docs*targets reorganized; R docs updated forread_A3json()default verbosity.
Reviewed changes
Copilot reviewed 10 out of 10 changed files in this pull request and generated 1 comment.
Show a summary per file
| File | Description |
|---|---|
| typescript/tests/schemas.test.ts | Updates/extends tests for sorted positions + duplicate rejection. |
| typescript/src/schemas.ts | Changes PositionsSchema to sort and reject duplicates via refinement. |
| rust/src/normalization.rs | Changes position normalization to error on duplicates; updates unit tests. |
| r/man/read_A3json.Rd | Updates documented default verbosity for read_A3json(). |
| python/rtemis_a3/tests/test_models.py | Updates model tests to expect duplicate position validation errors. |
| python/rtemis_a3/src/rtemis/a3/_normalize.py | Adds a dedicated duplicate-checking normalizer for strict parsing. |
| python/rtemis_a3/src/rtemis/a3/_models.py | Switches position normalization to duplicate-rejecting logic. |
| julia/RtemisA3/test/runtests.jl | Updates tests to expect duplicate positions to be rejected. |
| julia/RtemisA3/src/validate.jl | Updates validator to sort and reject duplicates. |
| Makefile | Moves/organizes docs targets earlier in the file; keeps summary behavior. |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
| @field_validator("index", mode="before") | ||
| @classmethod | ||
| def _normalize_positions(cls, v: Any) -> list[int]: | ||
| if not isinstance(v, list): | ||
| raise ValueError("index must be a list of positive integers") | ||
| return sort_dedup(v) | ||
| return check_no_duplicate_positions(v) | ||
|
|
There was a problem hiding this comment.
A3Position._normalize_positions runs in mode="before" and passes the raw list to check_no_duplicate_positions, which sorts the list. If a caller provides non-integer elements (e.g., strings/dicts), the sorted() call can raise a TypeError, producing a less helpful Pydantic ValidationError than the intended "index must be a list of positive integers"/type-specific messages. Consider moving this validator to mode="after" (so items are already validated/coerced as Position) or explicitly validating/coercing each element to an int (and rejecting bools) before sorting/checking duplicates.
![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 updates the position validation logic across Julia, Python, Rust, and TypeScript to explicitly reject duplicate entries instead of automatically deduplicating them. It also reorganizes the Makefile and updates the default verbosity in the R package documentation. The feedback highlights that the change to default verbosity in the R package appears unrelated to the primary goal of the PR and could impact existing users.
error on parse for duplicate indexes