Skip to content
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

Make sure docgen uses induced slots #408

Open
wants to merge 1 commit into
base: master
Choose a base branch
from
Open

Conversation

matentzn
Copy link
Collaborator

@matentzn matentzn commented Jan 6, 2025

Before, slot information was taken outside the context of a specific class (not induced), which led to wrong rendering in the docs.

Resolves #406

  • docs/ have been added/updated if necessary
  • make test has been run locally
  • tests have been added/updated (if applicable)
  • CHANGELOG.md has been updated.

If you are proposing a change to the SSSOM metadata model, you must

  • provide a full, working and valid example in examples/
  • provide a link to the related GitHub issue in the see_also field of the linkml model
  • provide a link to a valid example in the see_also field of the linkml model
  • run SSSOM-Py test suite against the updated model

Before, slot information was taken outside the context of a specific class (not induced), which led to wrong rendering in the docs.
@matentzn matentzn requested a review from sujaypatil96 January 6, 2025 07:10
Copy link
Contributor

@sujaypatil96 sujaypatil96 left a comment

Choose a reason for hiding this comment

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

The logic you've written here is partially correct, but iterating over c.slots is not as comprehensive as using class_slots() from SchemaView.

It might be better to switch over to using the class_induced_slots() method which i've linked out to in my review comments.

I'm happy to do it too, so let me know if you need help!

Comment on lines 52 to +53
{%- for slot in c.slots %}
{%- set slot_info = schemaview.get_slot(slot) %}
{%- set slot_info = schemaview.induced_slot(slot, c.name) %}
Copy link
Contributor

Choose a reason for hiding this comment

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

It looks like the logic in these lines and some lines above can be reduced using the class_induced_slots() method in SchemaView?

Like if you know the name of the class (mapping/mapping set) for which you want to retrieve a list of induced slots, then you can simply make calls to class_induced_slots() and pass mapping/mapping set as arguments.

Comment on lines 67 to +68
{%- for slot in c.slots %}
{%- set slot_info = schemaview.get_slot(slot) %}
{%- set slot_info = schemaview.induced_slot(slot, c.name) %}
Copy link
Contributor

Choose a reason for hiding this comment

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

Same comment as above for these lines as well.

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.

Contradiction in license Element Description for MappingSet Class
2 participants