Skip to content

schemaview.py: check for a valid slot def before calling a function #480

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.

Sign up for GitHub

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

Merged
merged 1 commit into from
Oct 23, 2025
Merged

schemaview.py: check for a valid slot def before calling a function #480

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
1 commit
Select commit
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
14 changes: 12 additions & 2 deletions linkml_runtime/utils/schemaview.py
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -465,8 +465,7 @@ def imports_closure(
# visit item
sn = todo.pop()
if sn not in self.schema_map:
imported_schema = self.load_import(sn)
self.schema_map[sn] = imported_schema
self.schema_map[sn] = self.load_import(sn)

# resolve item's imports if it has not been visited already
# we will get duplicates, but not cycles this way, and
Expand Down Expand Up @@ -1904,6 +1903,10 @@ def slot_applicable_range_elements(self, slot: SlotDefinition) -> list[ClassDefi
:param slot:
:return: list of element types
"""
if not slot or not isinstance(slot, SlotDefinition):
err_msg = "A SlotDefinition must be provided to generate the slot applicable range elements."
raise ValueError(err_msg)

Comment on lines +1906 to +1909
Copy link
Contributor

@Silvanoc Silvanoc Oct 22, 2025

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Wouldn't it make sense to have a function for this purpose, since the same validation is being applied in multiple places?

I mean something like

def _validate_slot(slot, err_sfx) -> None:
    if not slot or not isinstance(slot, SlotDefinition):
        raise ValueError(
            f"A SlotDefinition must be provided to generate the {err_sfx}."
        )

BTW you are typically optimizing. I don't get why you use the variable err_msg just for a single use.

Copy link
Contributor

@Silvanoc Silvanoc Oct 23, 2025

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Feel free to resolve comment, and merge, if desired

Copy link
Collaborator Author

@ialarmedalien ialarmedalien Oct 23, 2025 •
edited
Loading

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

the err_msg thing is to make the stack trace clearer -- otherwise you get an extra line:

https://docs.astral.sh/ruff/rules/raw-string-in-exception/

You are absolutely right about optimising by creating a generic function. I don't know why I didn't do it - end of the workday, maybe?

Copy link
Contributor

@Silvanoc Silvanoc Oct 23, 2025

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

You're right, I forgot that recommendation 😄

Copy link
Collaborator Author

@ialarmedalien ialarmedalien Oct 23, 2025

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I'm going to investigate options because as soon as I see a verification function like that, I want to make it more generic. I need to get my core work done and then I'll dedicate some time to thinking/researching argument validation.

is_any = False
range_types = []
for r in self.slot_range_as_union(slot):
Expand All @@ -1930,6 +1933,10 @@ def slot_range_as_union(self, slot: SlotDefinition) -> list[ElementName]:
:param slot:
:return: list of ranges
"""
if not slot or not isinstance(slot, SlotDefinition):
err_msg = "A SlotDefinition must be provided to generate the slot range as union."
raise ValueError(err_msg)

return list({y.range for y in [slot, *[x for x in [*slot.exactly_one_of, *slot.any_of] if x.range]]})

def induced_slot_range(self, slot: SlotDefinition, strict: bool = False) -> set[str | ElementName]: # noqa: FBT001, FBT002
Expand All @@ -1947,6 +1954,9 @@ def induced_slot_range(self, slot: SlotDefinition, strict: bool = False) -> set[
:return: set of ranges
:rtype: set[str | ElementName]
"""
if not slot or not isinstance(slot, SlotDefinition):
err_msg = "A SlotDefinition must be provided to generate the induced slot range."
raise ValueError(err_msg)

slot_range = slot.range
any_of_range = {x.range for x in slot.any_of if x.range}
Expand Down
35 changes: 35 additions & 0 deletions tests/test_utils/test_schemaview.py
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -2125,6 +2125,41 @@ def test_slot_range(
pytest.fail(f"Unexpected range_function value: {range_function}")


@pytest.mark.parametrize(
"slot_argument",
[
None,
"slot_name",
12345,
"",
set(),
{"this": "that"},
True,
False,
lambda x: f"slot {x}",
ClassDefinition(name="whatever"),
SlotDefinitionName("something"),
],
)
@pytest.mark.parametrize(
("range_function", "err_desc"),
[
("slot_range_as_union", "slot range as union"),
("induced_slot_range", "induced slot range"),
("slot_applicable_range_elements", "slot applicable range elements"),
],
)
def test_range_function_non_slot_input(
schema_view_core: SchemaView, slot_argument: Any, range_function: str, err_desc: str
) -> None:
"""Ensure that incorrect input to the range function generates an error message.

The schema content is not important as this is solely for testing errors when calling `range` functions with the wrong argument.
"""
with pytest.raises(ValueError, match=f"A SlotDefinition must be provided to generate the {err_desc}."):
getattr(schema_view_core, range_function)(slot_argument)


"""End of range-related tests. Phew!"""


Expand Down
Loading