Skip to content

Fix Schemaview.get_uri(...) for no default_prefix in schema #378

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

Open
wants to merge 3 commits into
base: main
Choose a base branch
from

Conversation

dalito
Copy link
Member

@dalito dalito commented Mar 24, 2025

upstream_repo: sneakers-the-rat/linkml
upstream_branch: python-3.13

Some test fail with upstream linkml but these tests seem wrong. They test for the wrong uris with a colon, for example here:

FAILED tests/test_utils/test_owl.py::test_pred_types - assert value matches snapshot /home/runner/work/linkml-runtime/linkml-runtime/linkml/tests/test_utils/__snapshots__/owl2.owl
  ----- Missing Triples -----
  
  <http://example.org/owl2/:annotslot> a owl:ObjectProperty,
          <https://w3id.org/linkml/SlotDefinition> ;
      rdfs:label "annotslot" ;
      rdfs:domain <http://example.org/owl2/:C1> ;
      rdfs:range <http://example.org/owl2/:C2> ;
      skos:inScheme <http://example.org/owl2> .

(...)

----- Added Triples -----
  
  <http://example.org/owl2/annotslot> a owl:ObjectProperty,
          <https://w3id.org/linkml/SlotDefinition> ;
      rdfs:label "annotslot" ;
      rdfs:domain <http://example.org/owl2/C1> ;
      rdfs:range <http://example.org/owl2/C2> ;
      skos:inScheme <http://example.org/owl2> .

This validates the presence of issue linkml/linkml#2578 but is not testing for the correct URIs.
(...or are we wrong in what we assume as correct URIs?)

Closes linkml/linkml#2578

@Silvanoc
Copy link
Contributor

I have added a regression test for this issue: #379. The test should fail as long as this PR hasn't been merged.

If you cherry-pick my commit there and put it onto yours in this PR, the newly added test should start succeeding. If you do it so, you can then close my PR. Otherwise we should merge mine after yours.

@dalito
Copy link
Member Author

dalito commented Mar 25, 2025

@Silvanoc This PR has already a test for the issue it solves. Do you think testing with a full schema (#379) provides a stronger safety net for future regressions?

@Silvanoc
Copy link
Contributor

@Silvanoc This PR has already a test for the issue it solves. Do you think testing with a full schema (#379) provides a stronger safety net for future regressions?

Sorry, my bad. I oversaw it.

As long as SchemaDefinition does not add a default_prefix "automagically", your test would suffice. You have tested that the test fails without your fix, right? Then I can close my PR.

@dalito
Copy link
Member Author

dalito commented Mar 25, 2025

Yes, I verified that the test reproduces the issue before adding the fix.

@dalito dalito force-pushed the issue2578-faulty-uri-gen branch 4 times, most recently from 4faf322 to 4e5fb09 Compare March 28, 2025 14:51
@dalito
Copy link
Member Author

dalito commented Mar 28, 2025

@sierra-moxon - #341 was not 3.13 compatible but merged. The introduced typing issues in test_schema_builder.py are also fixed in this PR.

@ptgolden
Copy link

ptgolden commented Mar 28, 2025

The remaining test failure is also from #341 not being compatible with the changes merged in #345. Specifically the changes in 43b3b3d. That change made it so that the __init__ function of dataclasses is no longer overridden so that they accept arbitrary args and kwargs. Previously, the expected error for additional args/kwargs was a ValueError raised in a __post_init__ method.

With that logic no longer in place, the expected error is a TypeError, which is what dataclasses normally throw in __init__ for unexpected args/kwargs.

Pretty sure the fix here is to just expect a TypeError here

with pytest.raises(ValueError):

@dalito dalito force-pushed the issue2578-faulty-uri-gen branch from 81c312c to df3bbc1 Compare March 28, 2025 19:29
Copy link

codecov bot commented Mar 28, 2025

Codecov Report

All modified and coverable lines are covered by tests ✅

Project coverage is 63.77%. Comparing base (2a8a08d) to head (df3bbc1).
Report is 38 commits behind head on main.

Additional details and impacted files
@@            Coverage Diff             @@
##             main     #378      +/-   ##
==========================================
+ Coverage   63.59%   63.77%   +0.18%     
==========================================
  Files          62       62              
  Lines        8916     8947      +31     
  Branches     2581     2588       +7     
==========================================
+ Hits         5670     5706      +36     
+ Misses       2637     2634       -3     
+ Partials      609      607       -2     

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.

🚀 New features to boost your workflow:
  • ❄️ Test Analytics: Detect flaky tests, report on failures, and find test suite problems.

sierra-moxon added a commit that referenced this pull request Mar 28, 2025
Cherry-pciked fixes from #378 for rc2
@dalito dalito force-pushed the issue2578-faulty-uri-gen branch from df3bbc1 to 154727a Compare April 12, 2025 13:26
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.

[owl]: erroneous ':' in generated URIs if no default_prefix
3 participants