Add (limited) multivalued support for multivalued SimpleDict values to pydanticgen / typescriptgen #2586
Conversation
There was a problem hiding this comment.
Pull Request Overview
This PR adds limited support for multivalued SimpleDict values to pydanticgen by updating the slot type generation logic and introducing a new test case.
- Introduces a new test (test_simpledict_with_list_value) to verify that multivalued SimpleDict slots generate the expected type signature.
- Updates the _inline_as_simple_dict_with_value method to handle multivalued slots by 8000 returning either a plain type or a list type based on the slot’s multivalued flag.
Reviewed Changes
Copilot reviewed 2 out of 2 changed files in this pull request and generated no comments.
| File | Description |
|---|---|
| tests/test_generators/test_pydanticgen.py | Adds a test case to ensure SimpleDict values marked as multivalued are wrapped appropriately in a Union and Dict. |
| linkml/generators/pydanticgen/pydanticgen.py | Adjusts the logic in _inline_as_simple_dict_with_value to return List[...] when a value slot is multivalued. |
Comments suppressed due to low confidence (1)
linkml/generators/pydanticgen/pydanticgen.py:761
- Consider adding an explicit check that 'value_slot_range_type' is not None before calling _get_pyrange to avoid potential errors if the type definition is missing.
value_slot_pyrange = _get_pyrange(value_slot_range_type, self.schemaview)
Codecov ReportAttention: Patch coverage is
Additional details and impacted files@@ Coverage Diff @@
## main #2586 +/- ##
==========================================
- Coverage 81.12% 0.35% -80.78%
==========================================
Files 121 119 -2
Lines 13477 13354 -123
Branches 3784 3769 -15
==========================================
- Hits 10933 47 -10886
- Misses 1917 13282 +11365
+ Partials 627 25 -602 ☔ View full report in Codecov by Sentry. 🚀 New features to boost your workflow:
|
There was a problem hiding this comment.
Pull Request Overview
This pull request adds limited multivalued support for SimpleDict values in pydanticgen and includes a new test to verify the behavior.
- Introduces a new test (test_simpledict_with_list_value) that validates that multivalued, inlined SimpleDict values are wrapped as a Dict with a Union type.
- Modifies the _inline_as_simple_dict_with_value method to differentiate between multivalued and non-multivalued cases by wrapping the type in a List appropriately.
Reviewed Changes
Copilot reviewed 2 out of 2 changed files in this pull request and generated no comments.
| File | Description |
|---|---|
| tests/test_generators/test_pydanticgen.py | Adds a new test to verify the handling of SimpleDict values with multivalued attributes |
| linkml/generators/pydanticgen/pydanticgen.py | Refactors _inline_as_simple_dict_with_value to correctly return a List-wrapped type when appropriate |
Comments suppressed due to low confidence (2)
tests/test_generators/test_pydanticgen.py:243
- [nitpick] The test assertion relies on an exact string match which could be brittle under minor formatting changes. Consider using a regex or a more flexible approach to verify the generated type structure.
assert "things: Optional[Dict[str, Union[List[str], Thing]]] = Field(default=None)" in code
linkml/generators/pydanticgen/pydanticgen.py:761
- Consider verifying that 'value_slot_range_type' is not None before calling _get_pyrange to avoid potential runtime exceptions if the type retrieval unexpectedly returns None.
value_slot_pyrange = _get_pyrange(value_slot_range_type, self.schemaview)
There was a problem hiding this comment.
Pull Request Overview
This PR adds (limited) multivalued support for SimpleDict values in both the TypeScript and Pydantic generators and includes new tests to verify the expected behavior.
- Updated TypeScript generator to handle multivalued slots by wrapping the pyrange in a List or a dictionary with an index signature.
- Updated Pydantic generator’s inline handling for SimpleDict values to support list values and added corresponding tests.
Reviewed Changes
Copilot reviewed 4 out of 4 changed files in this pull request and generated 1 comment.
| File | Description |
|---|---|
| tests/test_generators/test_typescriptgen.py | Added a new test to validate the TypeScript generator’s multivalued output. |
| tests/test_generators/test_pydanticgen.py | Added a new test ensuring the correct generation of multivalued SimpleDict values in Pydantic. |
| linkml/generators/pydanticgen/pydanticgen.py | Updated inline simple dict logic to handle list values when a slot is multivalued. |
| linkml/generators/typescriptgen.py | Added a new get_value_slot method and updated the range method to include its output. |
Comments suppressed due to low confidence (2)
linkml/generators/typescriptgen.py:207
- [nitpick] Ensure that the fallback behavior when 'value_slot' is None (returning a dictionary with index signature) is the intended handling for all cases; if multiple non-identifier slots might be present, additional logic or documentation might be needed.
if not value_slot:
tests/test_generators/test_pydanticgen.py:242
- [nitpick] Consider adding additional test cases to verify the behavior for both inlined and non-inlined multivalued slots to ensure comprehensive coverage.
assert "things: Optional[Dict[str, Union[List[str], Thing]]] = Field(default=None)" in code
Co-authored-by: Copilot <175728472+Copilot@users.noreply.github.com>
There was a problem hiding this comment.
I think this would be a good candidate for a refactor where we replace this method by nesting the template models. We're approaching a situation where we have one routine for rendering a slot using the templates and a different, parallel route for rendering the range of a slot when it's a class using string manipulation. I would really like to see this get pulled out into a template model and made declarative, because the existing method is already 6-nested-if-statements levels of opaque and adding more to that makes me nervous.
E.g. we could replace this and the outer code that calls it with something like
slot = PydanticAttribute(
# ... other stuff from slot_definition
range=ClassRange(cls=slot_definition.range)
)
class ClassRange(TemplateModel):
cls: ClassDefinition
@computed_field
def inlining_mode() -> L["list", "dict", "simpledict"] | None:
#. .. decide the inlining mode
@computed_field
def value_slot() -> SlotDefinition:
#... get the value slotAnd then a template like
#...
{% if inlining_mode == simpledict %}
dict[str, Union[{{ cls.camelcase_name }},
{%- if value_slot.multivalued -%}
list[{{ value_slot.range}}]
{%- else -%}
{{ value_slot.range }}
{%- endif -%}
]I didn't do this in my prior refactors because I wasn't exactly sure what it was supposed to do, which might be itself indicative of the need for a refactor.
|
I went ahead and tried the spot refactor suggested in my review comment. I'll be unavailable this afternoon, but can push it to a branch if th 8000 ere's interest. Here it is in patch form. Note I returned a tuple from the method for expediency. expand/collapse Patch for generate_slot_range() refactordiff --git a/linkml/generators/pydanticgen/pydanticgen.py b/linkml/generators/pydanticgen/pydanticgen.py
index 1f0df0d5..5c29e9aa 100644
--- a/linkml/generators/pydanticgen/pydanticgen.py
+++ b/linkml/generators/pydanticgen/pydanticgen.py
@@ -472,27 +472,7 @@ class PydanticGenerator(OOCodeGenerator, LifecycleMixin):
pyslot = PydanticAttribute(**slot_args)
pyslot = self.include_metadata(pyslot, slot)
- slot_ranges = []
- # Confirm that the original slot range (ignoring the default that comes in from
- # induced_slot) isn't in addition to setting any_of
- any_of_ranges = [a.range if a.range else slot.range for a in slot.any_of]
- if any_of_ranges:
- # list comprehension here is pulling ranges from within AnonymousSlotExpression
- slot_ranges.extend(any_of_ranges)
- else:
- slot_ranges.append(slot.range)
-
- pyranges = [self.generate_python_range(slot_range, slot, cls) for slot_range in slot_ranges]
-
- pyranges = list(set(pyranges)) # remove duplicates
- pyranges.sort()
-
- if len(pyranges) == 1:
- pyrange = pyranges[0]
- elif len(pyranges) > 1:
- pyrange = f"Union[{', '.join(pyranges)}]"
- else:
- raise Exception(f"Could not generate python range for {cls.name}.{slot.name}")
+ pyrange, slot_ranges = self.generate_slot_range(slot, cls)
pyslot.range = pyrange
@@ -519,7 +499,7 @@ class PydanticGenerator(OOCodeGenerator, LifecycleMixin):
else:
simple_dict_value = None
if len(slot_ranges) == 1:
- simple_dict_value = self._inline_as_simple_dict_with_value(slot)
+ simple_dict_value = self._inline_as_simple_dict_with_value(slot, cls)
if simple_dict_value:
# simple_dict_value might be the range of the identifier of a class when range is a class,
# so we specify either that identifier or the range itself
@@ -532,6 +512,31 @@ class PydanticGenerator(OOCodeGenerator, LifecycleMixin):
result.attribute.range = f"Optional[{result.attribute.range}]"
return result
+ def generate_slot_range(self, slot, cls):
+ slot_ranges = []
+ # Confirm that the original slot range (ignoring the default that comes in from
+ # induced_slot) isn't in addition to setting any_of
+ any_of_ranges = [a.range if a.range else slot.range for a in slot.any_of]
+ if any_of_ranges:
+ # list comprehension here is pulling ranges from within AnonymousSlotExpression
+ slot_ranges.extend(any_of_ranges)
+ else:
+ slot_ranges.append(slot.range)
+
+ pyranges = [self.generate_python_range(slot_range, slot, cls) for slot_range in slot_ranges]
+
+ pyranges = list(set(pyranges)) # remove duplicates
+ pyranges.sort()
+
+ if len(pyranges) == 1:
+ pyrange = pyranges[0]
+ elif len(pyranges) > 1:
+ pyrange = f"Union[{', '.join(pyranges)}]"
+ else:
+ raise Exception(f"Could not generate python range for {cls.name}.{slot.name}")
+
+ return pyrange, slot_ranges
+
@property
def predefined_slot_values(self) -> Dict[str, Dict[str, str]]:
"""
@@ -726,7 +731,7 @@ class PydanticGenerator(OOCodeGenerator, LifecycleMixin):
injected_classes = [textwrap.dedent(c) for c in injected_classes]
return injected_classes
- def _inline_as_simple_dict_with_value(self, slot_def: SlotDefinition) -> Optional[str]:
+ def _inline_as_simple_dict_with_value(self, slot_def: SlotDefinition, cls_def) -> Optional[str]:
"""
Determine if a slot should be inlined as a simple dict with a value.
@@ -759,7 +764,10 @@ class PydanticGenerator(OOCodeGenerator, LifecycleMixin):
value_slot = non_id_slots[0]
value_slot_range_type = self.schemaview.get_type(value_slot.range)
if value_slot_range_type is not None:
- value_slot_pyrange = _get_pyrange(value_slot_range_type, self.schemaview)
+ value_slot_pyrange, _ = self.generate_slot_range(
+ value_slot,
+ cls_def
+ )
if not value_slot.multivalued:
return value_slot_pyrange
else:
This passes the generator unit test and also the compliance tests, but I would recommend additional tests specifically for the expanded functionality. |
Adds a check for multivalued values of a SimpleDict range generated by pydanticgen, and wraps the pyrange in a List[] if the slot is multivalued, plus a test.