feat: add alternative combined anonymous instance identifier (#54)#62
feat: add alternative combined anonymous instance identifier (#54)#62Artifizer merged 1 commit intoGlobalTypeSystem:mainfrom
Conversation
📝 WalkthroughWalkthroughAdds formal support for a combined anonymous GTS identifier form: a type chain with a UUID tail (e.g., Changes
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~25 minutes Poem
🚥 Pre-merge checks | ✅ 2 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (2 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches
🧪 Generate unit tests (beta)
Comment |
There was a problem hiding this comment.
Actionable comments posted: 3
🧹 Nitpick comments (2)
tests/test_op3_id_parsing.py (1)
96-129: Consider adding assertions for UUID tail parsing.The test verifies basic parsing success (
body.ok=True,body.is_schema=False) but doesn't assert on the parsed UUID tail component. Other OP3 tests verify segment details likever_major,ver_minor, etc. Consider adding assertions that verify the UUID tail is correctly extracted, e.g.:
body.uuid_tailor equivalent field containing the parsed UUIDbody.segmentscorrectly excludes the UUID tailThis would provide stronger verification that the implementation correctly parses and separates the UUID tail from the type chain.
💡 Suggested additional assertions
teststeps = [ Step( RunRequest("parse id (combined anonymous instance)") .get("/parse-id") .with_params(**{"gts_id": "${id}"}) .validate() .assert_equal("status_code", 200) .assert_equal("body.id", "${id}") .assert_equal("body.ok", True) .assert_equal("body.is_schema", False) + # Optionally verify UUID tail parsing if the API exposes it + # .assert_equal("body.uuid_tail", "7a1d2f34-5678-49ab-9012-abcdef123456") ), ]🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@tests/test_op3_id_parsing.py` around lines 96 - 129, Add assertions to TestCaseTestOp3IdParsing_CombinedAnonymousInstance to verify the parsed UUID tail and that the segments exclude it: in the teststeps where RunRequest("parse id (combined anonymous instance)") is used, add assert_equal checks for the UUID field returned by the endpoint (e.g., "body.uuid_tail" or the actual key the API returns) and an assertion that "body.segments" (or the API's segments key) does not include that UUID tail; ensure these new assertions are placed alongside the existing .assert_equal("body.id", "${id}"), .assert_equal("body.ok", True), and .assert_equal("body.is_schema", False) validations so the test explicitly checks both extraction and exclusion of the UUID tail.tests/test_op2_schema_id_priority.py (1)
300-325: Addselected_entity_fieldassertion for parity with other OP2 tests.
This keeps expectations consistent with the rest of the suite.Proposed diff
assert body["schema_id"] == ( "gts.x.core.events.type.v1~" "x.commerce.orders.order_placed.v1.0~" ) + assert body["selected_entity_field"] == "id" assert body["selected_schema_id_field"] == "id"🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@tests/test_op2_schema_id_priority.py` around lines 300 - 325, In test_op2_combined_anonymous_id_takes_priority_over_explicit_type add an assertion that verifies the response includes the expected selected_entity_field (matching other OP2 tests); after the existing assertions for is_schema, id, schema_id, and selected_schema_id_field, assert body["selected_entity_field"] == "id" so the test checks parity with the suite's convention for entity-field selection.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In
`@examples/events/instances/gts.x.core.events.type_combined_id.v1`~x.commerce.orders.order_placed.v1.0~.examples.json:
- Around line 1-24: The example's "id" field uses the non-combined prefix
("gts.x.core.events.type.v1~...") which mismatches the file name indicating a
combined-id example; update the "id" value in the JSON (the id property inside
the example object) to use the combined-id prefix
"gts.x.core.events.type_combined.v1~x.commerce.orders.order_placed.v1.0~<uuid>"
(preserve the existing UUID or replace it with a valid one), or alternatively
rename the file to the non-combined schema if you intend to keep the current id
format.
In `@examples/events/schemas/gts.x.core.events.type_combined.v1`~.schema.json:
- Around line 55-60: The schema property "ingestedAt" is using the non-standard
"$readonly" keyword; update the "ingestedAt" property in the schema to use the
Draft‑07 compliant "readOnly" keyword (readOnly: true) instead of "$readonly" so
validators and tooling recognize the field as read-only; ensure no other
properties use "$readonly" and replace any occurrences with "readOnly".
In `@README.md`:
- Line 97: Fix the typo in the README table row that currently reads "Add
alternate combined anonymous instance identitfier format" by changing
"identitfier" to "identifier" so the row reads "Add alternate combined anonymous
instance identifier format"; update the exact table cell text to preserve
spacing and punctuation.
---
Nitpick comments:
In `@tests/test_op2_schema_id_priority.py`:
- Around line 300-325: In
test_op2_combined_anonymous_id_takes_priority_over_explicit_type add an
assertion that verifies the response includes the expected selected_entity_field
(matching other OP2 tests); after the existing assertions for is_schema, id,
schema_id, and selected_schema_id_field, assert body["selected_entity_field"] ==
"id" so the test checks parity with the suite's convention for entity-field
selection.
In `@tests/test_op3_id_parsing.py`:
- Around line 96-129: Add assertions to
TestCaseTestOp3IdParsing_CombinedAnonymousInstance to verify the parsed UUID
tail and that the segments exclude it: in the teststeps where RunRequest("parse
id (combined anonymous instance)") is used, add assert_equal checks for the UUID
field returned by the endpoint (e.g., "body.uuid_tail" or the actual key the API
returns) and an assertion that "body.segments" (or the API's segments key) does
not include that UUID tail; ensure these new assertions are placed alongside the
existing .assert_equal("body.id", "${id}"), .assert_equal("body.ok", True), and
.assert_equal("body.is_schema", False) validations so the test explicitly checks
both extraction and exclusion of the UUID tail.
…ypeSystem#54) Signed-off-by: Matt Garmon <matt.garmon@acronis.com>
There was a problem hiding this comment.
Actionable comments posted: 1
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@README.md`:
- Around line 1154-1156: The current chained-identifier regex literal allows
single-segment IDs because it doesn't require a tilde-separated second segment;
modify the regex shown (the long ^\s*gts... pattern) to include a positive
lookahead that ensures at least one tilde is present (e.g. add (?=.*~) after the
start anchor) so that strings without any '~' are rejected; update the README's
regex example accordingly.
Summary by CodeRabbit
New Features
Documentation
Tests