Conversation
There was a problem hiding this comment.
Code Review
This pull request implements mandatory envelope fields, $schema and a3_version, across the A3 format's R, Python, TypeScript, Julia, and Rust implementations. It includes a new JSON schema, updated specifications, and corresponding validation and serialization logic across all supported languages. Feedback recommends using package-level constants in R for better maintainability and utilizing .pop() in Python for more efficient dictionary manipulation when stripping envelope keys.
| .a3_schema_uri <- "https://schema.rtemis.org/a3/v1/schema.json" | ||
| .a3_version <- "1.0.0" |
There was a problem hiding this comment.
The schema URI and version strings are hardcoded here and also in the to_json method (lines 887-888). To improve maintainability and ensure consistency across the package, these should be defined as package-level constants (e.g., .A3_SCHEMA_URI and .A3_VERSION) at the top of the file or in a central configuration file.
| f"'a3_version' must be '{_A3_VERSION}', got '{version_val}'" | ||
| ) | ||
| # Strip envelope keys before passing to the data model | ||
| data = {k: v for k, v in data.items() if k not in _ENVELOPE_KEYS} |
There was a problem hiding this comment.
Creating a new dictionary via a comprehension to strip envelope keys is functional but slightly less efficient than in-place modification, especially for larger objects. Since data is a fresh dictionary returned by json.loads, you can use .pop() to remove the keys in-place.
| data = {k: v for k, v in data.items() if k not in _ENVELOPE_KEYS} | |
| for key in _ENVELOPE_KEYS: | |
| data.pop(key, None) |
There was a problem hiding this comment.
Pull request overview
This PR standardizes the A3 “envelope” across languages by requiring $schema and a3_version on JSON input, emitting them on serialization, and adding a published JSON Schema at https://schema.rtemis.org/a3/v1/schema.json.
Changes:
- Enforce
$schema+a3_versionvalidation across TypeScript, Python, R, Julia, and Rust implementations and update tests/examples accordingly. - Add a JSON Schema document (
schema/a3.schema.json) and update the spec docs to describe the envelope fields. - Add a root
Makefileto run tests/docs across the multi-language repo.
Reviewed changes
Copilot reviewed 20 out of 20 changed files in this pull request and generated 5 comments.
Show a summary per file
| File | Description |
|---|---|
| typescript/tests/schemas.test.ts | Updates schema validation tests to include envelope fields. |
| typescript/tests/a3.test.ts | Updates A3 class tests to include envelope fields in some constructors/serialization checks. |
| typescript/src/schemas.ts | Adds exported envelope constants and requires $schema/a3_version in A3InputSchema. |
| typescript/src/a3.ts | Ensures serialized output includes envelope fields first. |
| specs/A3.md | Documents $schema and a3_version, updates implementation-note links, and adds .a3.json extension note. |
| specs/A3_Rust.md | Renames Rust spec title/wording. |
| schema/a3.schema.json | Introduces JSON Schema for A3 v1. |
| rust/src/validation.rs | Adds envelope field validation and propagates validated envelope fields into normalized output. |
| rust/src/types.rs | Adds $schema/a3_version fields to the Rust A3 type and constants for expected values. |
| rust/src/lib.rs | Updates docs/tests and adds tests for missing/wrong envelope fields. |
| r/tests/testthat/test_A3.R | Updates JSON fixtures to include envelope fields. |
| r/R/a3.R | Emits envelope fields in to_json() and validates them in A3from_json(). |
| python/rtemis_a3/tests/test_api.py | Updates API tests for envelope requirements and adds missing-field tests. |
| python/rtemis_a3/src/rtemis/a3/api.py | Validates envelope fields in a3_from_json() and emits them in a3_to_json(). |
| Makefile | Adds multi-language test and docs targets. |
| julia/RtemisA3/test/runtests.jl | Updates tests for envelope requirements. |
| julia/RtemisA3/src/validate.jl | Validates envelope fields and allows them as recognized top-level keys. |
| julia/RtemisA3/src/io.jl | Emits envelope fields in dictionary serialization. |
| julia/RtemisA3/src/api.jl | Includes envelope fields in create_a3() raw dict. |
| examples/mapt.a3.json | Adds envelope fields to example A3 JSON. |
Comments suppressed due to low confidence (1)
specs/A3.md:173
- In the Stage 1 validation bullets, the ranges line says "overlaps merged", but earlier in this same spec (Ranges definition) and in all language implementations, overlapping ranges are rejected (not merged). This looks like a spec inconsistency and could mislead implementers; consider updating the bullet to match the actual behavior (reject overlaps).
- `$schema`: required string; must equal `"https://schema.rtemis.org/a3/v1/schema.json"`
- `a3_version`: required string; must equal `"1.0.0"`
- `sequence`: non-empty, `[A-Za-z*]+` (uppercased on parse), ≥ 2 characters
- Positions: positive integers, sorted, deduplicated
- Ranges: positive integers, `start < end`, sorted, overlaps merged
- Annotation entries: `{ index, type }` objects — bare arrays rejected
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
| "type": "object", | ||
| "required": ["$schema", "a3_version", "sequence", "annotations", "metadata"], | ||
| "additionalProperties": false, |
There was a problem hiding this comment.
The JSON Schema currently marks annotations and metadata as required at the top level. This conflicts with the spec and implementations in this repo that default missing annotations/metadata on input (e.g., TypeScript A3InputSchema defaults both, and Python model defaults both). As a result, minimal JSON like { "$schema": ..., "a3_version": ..., "sequence": ... } is accepted by libraries but would be rejected by this schema. Consider making annotations and metadata optional here (and similarly relaxing nested requirements) so $schema validation matches what parsers accept.
| }, | ||
| "annotations": { | ||
| "type": "object", | ||
| "required": ["site", "region", "ptm", "processing", "variant"], |
There was a problem hiding this comment.
Within annotations, the schema requires all five families (site, region, ptm, processing, variant). Repo implementations accept partial annotations objects and default missing families to empty (e.g., TS tests assert this behavior). To keep the JSON Schema aligned with accepted input, consider removing/relaxing this required list (or otherwise making the families optional with defaults).
| "required": ["site", "region", "ptm", "processing", "variant"], |
| it("serialized JSON contains all annotation families", () => { | ||
| const a3 = new A3({ sequence: "MKTAY" }); | ||
| const a3 = new A3({ | ||
| $schema: "https://schema.rtemis.org/a3/v1/schema.json", |
There was a problem hiding this comment.
Now that $schema/a3_version are mandatory, several tests in this file still construct invalid inputs without the envelope (e.g., earlier new A3({ sequence: ... }) cases). Those tests will still throw, but for the wrong reason (missing envelope instead of the intended validation like "too short" or "out of bounds"), reducing coverage. Consider updating those invalid-input tests to include the correct envelope so they continue to exercise the specific validation being asserted.
| @@ -166,6 +172,8 @@ describe("A3.toJSON and JSON.stringify", () => { | |||
|
|
|||
| it("type field is always present on annotation entries", () => { | |||
There was a problem hiding this comment.
The schema URI and version are now duplicated as string literals in multiple places in this file. Since typescript/src/schemas.ts exports A3_SCHEMA_URI and A3_VERSION, consider importing and reusing those constants in tests to avoid future drift if the URI/version ever changes.
| $schema: "https://schema.rtemis.org/a3/v1/schema.json", | ||
| a3_version: "1.0.0", | ||
| sequence: "MKTAYIAKQR", |
There was a problem hiding this comment.
This test hardcodes the schema URI and version inline while the file already has MINIMAL_VALID (and schemas.ts exports A3_SCHEMA_URI/A3_VERSION). Consider reusing a shared constant/object here to reduce duplication and the chance of the URI/version drifting across tests.
| $schema: "https://schema.rtemis.org/a3/v1/schema.json", | |
| a3_version: "1.0.0", | |
| sequence: "MKTAYIAKQR", | |
| $schema: MINIMAL_VALID.$schema, | |
| a3_version: MINIMAL_VALID.a3_version, | |
| sequence: MINIMAL_VALID.sequence, |
No description provided.