Conversation
There was a problem hiding this comment.
Code Review
This pull request implements a significant change in the A3 format validation logic across multiple languages (Julia, Python, R, Rust, TypeScript), moving from automatic deduplication of positions to rejecting duplicate positions with a validation error. It also includes extensive code formatting updates and documentation revisions to reflect this change. Review feedback identified an invalid version update for the "colored" crate in Rust, an incorrect Zod version reference in the documentation, and a typing issue in the TypeScript Zod schema where the input type was overly restricted, potentially causing issues with data transformations.
| [dependencies] | ||
| clap = { version = "4", features = ["derive"] } | ||
| colored = "2" | ||
| colored = "3" |
There was a problem hiding this comment.
| - Formatter / linter: `biome` | ||
| - Test runner: `vitest` | ||
| - Validation: `zod ^3` | ||
| - Validation: `zod ^4` |
There was a problem hiding this comment.
| // ── Root schema ─────────────────────────────────────────────────────────────── | ||
|
|
||
| export const A3InputSchema = z | ||
| export const A3InputSchema: z.ZodType<A3Data> = z |
There was a problem hiding this comment.
Typing A3InputSchema as z.ZodType<A3Data> incorrectly sets the schema's input type to match its output type (A3Data). In Zod, z.ZodType<T> is a shorthand for z.ZodType<T, z.ZodTypeDef, T>. Since this schema includes transforms (e.g., uppercasing the sequence, sorting arrays) and defaults, the expected input is more lenient than the A3Data interface. This can lead to TypeScript errors when passing raw data to the schema or when using z.input<typeof A3InputSchema>. It is recommended to explicitly set the input type to any or unknown.
| export const A3InputSchema: z.ZodType<A3Data> = z | |
| export const A3InputSchema: z.ZodType<A3Data, z.ZodTypeDef, any> = z |
| export type A3PositionData = z.infer<typeof A3PositionSchema>; | ||
| export type A3RangeData = z.infer<typeof A3RangeSchema>; | ||
| export type A3FlexData = z.infer<typeof A3FlexSchema>; | ||
| }) as z.ZodType<A3Data>; |
There was a problem hiding this comment.
Pull request overview
Aligns the A3 schema implementations and multi-language design docs around stricter handling of duplicate positions (reject rather than deduplicate), and updates related implementation notes across TypeScript, Rust, Python, R, and Julia.
Changes:
- TypeScript: move to explicitly exported interfaces for A3 data shapes and update Zod schema behavior/docs to reject duplicate positions.
- Specs: update A3 format and implementation design documents (Zod, S7/R, Rust, Pydantic, Julia) to reflect “duplicates rejected” semantics and related normalization rules.
- Implementations/tests: small formatting/refactor updates across Rust/Python/R/Julia, plus Rust crate version/dependency bump.
Reviewed changes
Copilot reviewed 21 out of 24 changed files in this pull request and generated 4 comments.
Show a summary per file
| File | Description |
|---|---|
| typescript/src/schemas.ts | Exports A3 envelope constants + explicit TS interfaces; updates Zod validation to reject duplicate positions and enforce non-overlapping ranges. |
| specs/A3_zod.md | Updates Zod/TypeScript design notes for Zod v4 and “duplicates rejected” behavior. |
| specs/A3_S7.md | Updates S7 (R) design notes to clarify duplicate positions are rejected. |
| specs/A3_Rust.md | Updates Rust design notes to reflect duplicate rejection semantics. |
| specs/A3_Pydantic.md | Updates Pydantic design notes to reflect duplicate rejection semantics and validator behavior. |
| specs/A3_Julia.md | Updates Julia design notes to reflect duplicate rejection semantics. |
| specs/A3.md | Updates core A3 spec text to state duplicates are rejected (not removed). |
| rust/src/normalization.rs | Updates spec-aligned docs for normalization rules (duplicates rejected). |
| rust/src/diagnostic.rs | Documentation formatting tweaks in diagnostic module header comments. |
| rust/Cargo.toml | Bumps crate version and updates a dependency version. |
| r/tests/testthat/test_A3.R | Formatting-only adjustments in R tests. |
| python/rtemis_a3/tests/test_models.py | Minor formatting in a test case. |
| python/rtemis_a3/tests/test_api.py | Minor formatting in JSON parsing tests. |
| python/rtemis_a3/src/rtemis/a3/api.py | Formatting/refactor of error construction and envelope validation error payloads. |
| python/rtemis_a3/src/rtemis/a3/_normalize.py | Minor formatting in an error message. |
| python/rtemis_a3/src/rtemis/a3/_models.py | Minor formatting/line-wrapping adjustments in validators and errors. |
| julia/RtemisA3/test/runtests.jl | Formatting refactors and expanded validation tests around duplicate rejection/overlap checks. |
| julia/RtemisA3/src/validate.jl | Formatting refactors; maintains duplicate rejection + bounds checks. |
| julia/RtemisA3/src/types.jl | Formatting of explicit equality implementations. |
| julia/RtemisA3/src/normalize.jl | Formatting refactors in normalization helpers. |
| julia/RtemisA3/src/io.jl | Formatting refactors in JSON serialization/deserialization helpers. |
| julia/RtemisA3/src/errors.jl | Formatting refactor in showerror definitions. |
| julia/RtemisA3/src/api.jl | Formatting refactor in create_a3 API. |
| julia/RtemisA3/docs/make.jl | Formatting refactor in Documenter configuration. |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
| export const A3InputSchema: z.ZodType<A3Data> = z | ||
| .object({ |
There was a problem hiding this comment.
A3InputSchema is typed as z.ZodType<A3Data>, which implies the schema’s input type is also A3Data. Since this schema accepts unnormalized inputs (e.g. lowercase sequence, unsorted index) and applies transforms, it’s more accurate to type it with an unknown (or a dedicated input interface) input type (e.g. ZodType<A3Data, …, unknown>). This avoids misleading downstream type inference (e.g. z.input<typeof A3InputSchema> becoming A3Data).
| export type A3PositionData = z.infer<typeof A3PositionSchema>; | ||
| export type A3RangeData = z.infer<typeof A3RangeSchema>; | ||
| export type A3FlexData = z.infer<typeof A3FlexSchema>; | ||
| }) as z.ZodType<A3Data>; |
There was a problem hiding this comment.
The final as z.ZodType<A3Data> cast hides type mismatches introduced by the schema’s transforms/effects (e.g. .transform, .superRefine). Prefer adjusting the schema’s generic parameters (including the input type) so it type-checks without a cast, rather than asserting the type here.
| for (i, p) in enumerate(entry.index) | ||
| ; | ||
| chk(p, "annotations.ptm.$name.index[$i]"); | ||
| end | ||
| else | ||
| for (i, r) in enumerate(entry.index); chk_range(r, "annotations.ptm.$name.index[$i]"); end | ||
| for (i, r) in enumerate(entry.index) | ||
| ; | ||
| chk_range(r, "annotations.ptm.$name.index[$i]"); | ||
| end | ||
| end | ||
| end | ||
| for (name, entry) in annotations.processing | ||
| if entry isa A3Position | ||
| for (i, p) in enumerate(entry.index); chk(p, "annotations.processing.$name.index[$i]"); end | ||
| for (i, p) in enumerate(entry.index) | ||
| ; | ||
| chk(p, "annotations.processing.$name.index[$i]"); |
There was a problem hiding this comment.
These standalone ; lines (and the trailing ; after function calls) are non-idiomatic and make the control flow harder to read. They don’t appear to serve a purpose here and should be removed to keep the bounds-check loops straightforward.
| for (i, r) in enumerate(entry.index) | ||
| ; | ||
| chk_range(r, "annotations.processing.$name.index[$i]"); | ||
| end |
There was a problem hiding this comment.
Same as above: the ;-only line and trailing semicolons in this range branch are unnecessary and reduce readability; please remove them.
No description provided.