Skip to content

Fix CopyDirective exclude/exclude_all on slot-list classes#244

Open
amc-corey-cox wants to merge 4 commits into
mainfrom
fix-copy-directive-list
Open

Fix CopyDirective exclude/exclude_all on slot-list classes#244
amc-corey-cox wants to merge 4 commits into
mainfrom
fix-copy-directive-list

Conversation

@amc-corey-cox
Copy link
Copy Markdown
Contributor

The _copy_list path in SchemaMapper (used when a source class declares its slots as a list rather than inline attributes) had two bugs that made exclude and exclude_all non-functional:

  • exclude checked the directive's truthiness instead of membership, so it removed every source slot.
  • exclude_all mutated the list while iterating, leaving roughly half the elements intact.

Mirrors _copy_dict's behavior (iterate the exclude list, check membership in the target) and uses list.clear() for exclude_all. Adds parametrized tests covering exclude, exclude_all, include, and combinations on the slot-list path — none of this was previously covered.

Closes #67.

Two related items deliberately left out of scope, to be filed as follow-ups:

  • CopyDirective.add slot is declared in the YAML schema but has no description and no implementation. Needs design.
  • Mechanism for overriding/clearing individual attributes of a copied slot (the use case described in implement the exclude and include slots in the CopyDirective class #67's body) is a different feature, likely belonging on SlotDerivation.target_definition.

The _copy_list path (used when a source class declares slots as a list
rather than inline attributes) had two bugs:
- `exclude` checked the directive's truthiness instead of membership,
  so it removed every source slot.
- `exclude_all` mutated the list while iterating, leaving half intact.

Mirror _copy_dict's behavior and clear() the list. Add parametrized
tests covering exclude, exclude_all, include, and combinations on the
slot-list path.

Closes #67
Copy link
Copy Markdown
Contributor

Copilot AI left a comment

Choose a reason for hiding this comment

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

Pull request overview

Fixes SchemaMapper._copy_list so CopyDirective.exclude and exclude_all behave correctly when copying slot-list classes (i.e., classes that declare slots: [...] rather than inline attributes). Adds regression tests to cover these directive behaviors on the slot-list path (closing #67).

Changes:

  • Correct exclude logic in _copy_list to remove only specified elements rather than removing everything.
  • Fix exclude_all logic in _copy_list by clearing the target list safely.
  • Add parametrized tests covering copy_all, exclude, exclude_all, and include for slot-list classes.

Reviewed changes

Copilot reviewed 2 out of 2 changed files in this pull request and generated no comments.

File Description
src/linkml_map/inference/schema_mapper.py Fixes CopyDirective handling for list-based slot copying (exclude / exclude_all).
tests/test_schema_mapper/test_schema_mapper.py Adds regression tests for CopyDirective behavior on slot-list classes.
Comments suppressed due to low confidence (1)

src/linkml_map/inference/schema_mapper.py:92

  • In _copy_list, the include step appends elements unconditionally. If a directive uses copy_all=True (or multiple directives run) and also sets include, this will duplicate slot names in tgt_elements, which can make the derived schema invalid or at least surprising. Consider making list-copy idempotent by only appending when the element is not already present (or by maintaining an order-preserving set) during copy_all/include operations.
            tgt_elements.clear()
        if copy_directive.include:
            for element in copy_directive.include:
                if element in src_elements:
                    tgt_elements.append(element)

The slot-list path appends unconditionally, so `copy_all=True` combined
with `include=[...]` (or any combination where the same name is reached
twice) produces duplicate slot names in the target class.

Guard both branches with a membership check. Add a parametrized case
covering copy_all + include.
Copilot AI review requested due to automatic review settings May 14, 2026 20:13
Copy link
Copy Markdown
Contributor

Copilot AI left a comment

Choose a reason for hiding this comment

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

Pull request overview

Copilot reviewed 2 out of 2 changed files in this pull request and generated 1 comment.

Comment on lines 80 to +83
if copy_directive.copy_all:
for element in src_elements:
tgt_elements.append(element)
if element not in tgt_elements:
tgt_elements.append(element)
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.

implement the exclude and include slots in the CopyDirective class

2 participants