Skip to content

feat: carry each chart mapping's framework - #226

Merged
jfrench9 merged 1 commit into
mainfrom
chore/mapping-framework-fields
Sep 30, 2026
Merged

jfrench9 merged 1 commit into
mainfrom
chore/mapping-framework-fields

Conversation

@jfrench9

@jfrench9 jfrench9 commented Sep 30, 2026 •

Copy link
Copy Markdown
Member

Summary

Picks up robosystems#1608, which anchors each chart mapping to a mapping taxonomy the chart owns and records the framework it maps into. The regenerated models carry the new optional field, and LedgerClient now returns each mapping's framework.

Release order: publish only after robosystems#1608 is deployed. list_mappings and list_structures now select framework, and a server without that field rejects the query.

Changes

  • Regenerated REST models (robosystems_client/models/): TaxonomyBlockStructureRequest and TaxonomyBlockStructure gain an optional target_framework, used only on coa_mapping structures; omitted means the book framework. The regen picked up only this change, with no other drift since the last generation.
  • GraphQL (robosystems_client/graphql/):
    • schema.graphql is refreshed; the Structure type gains framework.
    • The ListLedgerMappings and ListLedgerStructures operations select it.
    • generated/ is regenerated from those operations.
  • Facade (clients/ledger_client.py):
    • list_mappings and list_structures return framework.
    • The list_mappings docstring says the book mapping is listed first.
    • Both are sync-only facade methods; there is no async twin to cover.
  • Tests (tests/test_ledger_client.py): the two fixtures carry framework, and list_mappings asserts it.

Compatibility

ADDITIVE: a client minor. The new REST model fields are optional, and framework is a new, nullable field on two facade return types. No export, signature or existing field changed.

Deploy-order dependency: the two facade queries request a field that only exists once robosystems#1608 is deployed. Against an older server they fail GraphQL validation, so this has to be published after that deploy.

Testing

  • Regenerated with just generate-sdk against the local stack running robosystems#1608's branch. The branch was current with main, and the new fields were confirmed in the live /openapi.json and GraphQL schema first.
  • just generate-graphql was run after the operation edits.
  • just test-all: 595 passed, 17 skipped; ruff and format clean; basedpyright 0 errors.
  • just build-package was not run (no packaging change).

🤖 Generated with Claude Code

Regenerated against robosystems#1608, which anchors each coa_mapping
structure to a mapping taxonomy the chart owns:

- TaxonomyBlockStructureRequest / TaxonomyBlockStructure gain an optional
  target_framework (coa_mapping only)
- the GraphQL Structure type gains framework; LedgerClient.list_mappings
  and list_structures now return it, the book mapping listed first

Claude-Session: https://claude.ai/code/session_01HdEHDANCXBjrPfKagNDgSB
@jfrench9
jfrench9 marked this pull request as ready for review September 30, 2026 23:31
@jfrench9

Copy link
Copy Markdown
Member Author

@claude please review this PR

@claude

claude Bot commented Sep 30, 2026 •

Copy link
Copy Markdown

Claude finished @jfrench9's task in 58s —— View job


Review complete

  • Gather context (PR description, changed files)
  • Read repo CLAUDE.md for conventions (none exists in this repo)
  • Review diff against origin/main
  • Check regenerated models/GraphQL for consistency
  • Check facade + tests
  • Post review feedback

Verdict: LGTM. This is a clean, well-scoped additive change.

What I checked:

  • schema.graphql / Structure.framework is correctly documented as coa_mapping-only and nullable, matching ListLedgerMappings.graphql and ListLedgerStructures.graphql, which are the only two operations that select the shared id name description blockType taxonomyId isActive field set — no other operation was missed.
  • The generated Pydantic models (list_ledger_mappings.py, list_ledger_structures.py) add framework: Optional[str] with no alias, consistent with the existing description: Optional[str] field — codegen output looks correctly regenerated, not hand-edited.
  • TaxonomyBlockStructure / TaxonomyBlockStructureRequest REST models gain target_framework as None | str | Unset, following the exact to_dict/from_dict pattern used for the neighboring role_uri field. Consistent and additive.
  • ledger_client.py: list_mappings docstring now documents both the framework field and the "book mapping first" ordering — worth double-checking that claim is actually guaranteed server-side (robosystems#1608) since nothing here enforces or tests that ordering; if it's not a hard guarantee it may be worth softening the wording.
  • Tests: both fixtures (list_mappings, list_structures) were updated with framework, and only list_mappings asserts on the new value, which matches the facade surface described in the PR body. The incidental taxonomyId fixture change (tax_usgaap → tax_map_gaap) in test_list_mappings_unwraps_structures is harmless — nothing else in the test depends on that value.
  • Confirmed no async twin exists for list_mappings/list_structures in ledger_client.py, so the "sync-only, no async twin" claim in the PR body checks out.
  • Deploy-order dependency is clearly called out in the PR body; nothing in the diff undermines that (both new GraphQL selections are unconditional, so they will indeed fail validation against a server without the field, as stated).

Nit (non-blocking): the list_mappings docstring's "book mapping first" claim reads as an API guarantee but isn't covered by a test — consider adding an assertion (or at least a comment on the mock ordering) once robosystems#1608 lands, or softening the docstring if ordering isn't contractually guaranteed.

Note: I was unable to run just test-all/ruff/basedpyright in this sandbox (shell commands required approval that wasn't available), so I relied on static review of the diff. The PR description states just test-all passed locally (595 passed, 17 skipped; ruff/format/basedpyright clean), which is consistent with what I see in the diff.

@jfrench9
jfrench9 merged commit 035d372 into main Sep 30, 2026
4 checks passed
@jfrench9
jfrench9 deleted the chore/mapping-framework-fields branch September 30, 2026 23:36
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

1 participant