-
Notifications
You must be signed in to change notification settings - Fork 42
Refactor registry construction #844
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Refactor registry construction #844
Conversation
- use actual object classes instead of generic `object` inside the registry - reduce dependency on direct import of `ssvc.registry.REGISTRY` object in favor of `get_registry()` method to improve late-binding and avoid circular imports - use `model_post_init()` for registerable objects
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Pull Request Overview
This PR refactors the SSVC registry construction by replacing generic object placeholders with specific typed objects, introducing a get_registry() accessor function, and modernizing the registration mechanism.
- Replaces generic
objecttype in registry withUnion[DecisionPoint, DecisionTable]for proper type enforcement - Introduces
get_registry()function to replace directREGISTRYimports, preventing circular import issues - Updates registration mechanism from
@model_validatordecorators tomodel_post_init()hooks
Reviewed Changes
Copilot reviewed 15 out of 15 changed files in this pull request and generated 2 comments.
Show a summary per file
| File | Description |
|---|---|
src/ssvc/registry/base.py |
Major refactor of registry classes with proper typing and function extraction |
src/ssvc/registry/__init__.py |
Replaces global REGISTRY with get_registry() function |
src/ssvc/_mixins.py |
Adds _Registered mixin with model_post_init() registration |
src/ssvc/decision_points/base.py |
Updates to use _Registered mixin and removes old registration pattern |
src/ssvc/decision_tables/base.py |
Updates to use _Registered mixin and get_registry() function |
data/schema/v2/Ssvc_Object_Registry-2-0-0.schema.json |
Updated JSON schema with explicit type definitions |
| Multiple test files | Updated to use get_registry() function and new registry patterns |
Co-authored-by: Copilot <[email protected]>
Co-authored-by: Copilot <[email protected]>
|
replying to #844 (comment)
At a I think this is okay because it allows us to build what we used to call "compound decision points" as just extra SSVC/src/ssvc/decision_tables/ssvc/utlity.py Lines 32 to 62 in 1fb7e2a
and then how just SSVC/src/ssvc/decision_tables/ssvc/supplier_dt.py Lines 35 to 60 in 1fb7e2a
We previously had thought that this was just a notational convenience. Which it is, but it's useful to be able to model it this way (with two distinct So I don't think there's really much of a reason to distinguish "outcomes" from "decision points" in the |
|
@sei-vsarvepalli I added a I think this is as ready to go as it needs to be unless you have any blockers. |
sei-vsarvepalli
left a comment
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
All features check out!
This PR extends the registry object definition added in #795.
Replace generic
objectplaceholder in object with specific defined objectsMost significantly, it removes a kludge in which the
SsvcObjectRegistryused generic pythonobjectas the data type for theobjtype inside a version branch. Now we explicitly defineobjas aUnion[DecisionPoint,DecisionTable]to enforce pydantic typing all the way down.Side-effect: updated Registry schema.json
This change has side effects on the json schema for the registry object, notably that it now includes the definitions of both objects in the schema. Because the json schemas are entirely generated, understanding the python
SsvcObjectRegistryclass is important to make sense of the schema.Incidentals
REGISTRYobject in favor of aget_registry()method that returns the object. This ensures consistent runtime-binding without risk of circular imports. Also guarantees that the registry has been initiated consistently at first use.model_post_init()hooks instead of@model_validatordecorators for side effects. Specifically, we now initiate object registration from within themodel_post_init()instead of having it be set up as if it were a validator.Copilot Summary
This pull request introduces significant refactoring and enhancements to the SSVC object registry schema and the registry access pattern throughout the codebase. The main changes include a major redesign of the JSON schema for registry objects, the introduction of a
get_registry()accessor to replace direct global registry usage, and updates to the registration and validation logic for decision points and decision tables.Key changes:
Schema Redesign and Registry Model
Ssvc_Object_Registry-2-0-0.schema.json: introduces new definitions forDecisionPoint,DecisionPointValue, andDecisionTableobjects, restructures the registry to use internal types (_Key,_Namespace,_Version,_NsType), and updates required fields and validation patterns for keys, namespaces, and versions. The schema now supports more flexible and explicit modeling of decision points, tables, and their values. [1] [2] [3] [4] [5] [6]Registry Access and Usage
REGISTRYvariable with theget_registry()function throughout the codebase, improving encapsulation and testability. This affectssrc/ssvc/__init__.py,src/ssvc/doctools.py, and several internal utility functions insrc/ssvc/decision_tables/base.py. [1] [2] [3] [4] [5] [6] [7]Decision Point and Decision Table Enhancements
model_post_inithook instead of the deprecated@model_validator(mode="after"), ensuring objects are registered immediately after initialization. [1] [2]decision_points/base.pyanddecision_tables/base.pyto support the new registry pattern and improve type safety. [1] [2] [3] [4]These changes modernize the SSVC registry infrastructure, improve schema validation, and make registry usage more robust and maintainable.