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

Improve curie validation #285

Draft
wants to merge 4 commits into
base: main
Choose a base branch
from
Draft

Improve curie validation #285

wants to merge 4 commits into from

Conversation

Silvanoc
Copy link
Contributor

@Silvanoc Silvanoc commented Nov 8, 2023

Removing wrong expectations on CURIEs and adding new expectations. Also fixing validation according the specification.

According the CURIE syntax specification [1] "The prefix is separated
from the reference by a colon (:). It is possible to omit both the
prefix and the colon [...]." But a test case existed expecting a CURIE
omitting both prefix and colon to be invalid.

This patch fixes this wrong expectation.

[1]: https://www.w3.org/TR/curie/#s_syntax

Signed-off-by: Silvano Cirujano Cuesta <[email protected]>
CURIE relative_ref part can perfectly be a number. Fixing wrong
expectation rejecting number only relative_refs.

Signed-off-by: Silvano Cirujano Cuesta <[email protected]>
CURIE prefixes can start with an underscore ('_'). This patch adds a
test case to validate it.

Signed-off-by: Silvano Cirujano Cuesta <[email protected]>
@Silvanoc Silvanoc marked this pull request as draft November 8, 2023 01:08
CURIE validation in metamodelcore was erroneous and rejecting valid
CURIEs like those missing a prefix and a colon. This patch fixes it.

Signed-off-by: Silvano Cirujano Cuesta <[email protected]>
Copy link

codecov bot commented Nov 8, 2023

Codecov Report

Attention: 2 lines in your changes are missing coverage. Please review.

Comparison is base (d45970a) 62.09% compared to head (cf960f0) 62.08%.
Report is 1 commits behind head on main.

Additional details and impacted files
@@            Coverage Diff             @@
##             main     #285      +/-   ##
==========================================
- Coverage   62.09%   62.08%   -0.01%     
==========================================
  Files          63       63              
  Lines        8461     8459       -2     
  Branches     2170     2169       -1     
==========================================
- Hits         5254     5252       -2     
  Misses       2599     2599              
  Partials      608      608              
Files Coverage Δ
linkml_runtime/utils/metamodelcore.py 78.26% <50.00%> (-0.18%) ⬇️

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

@Silvanoc Silvanoc changed the title Improve curie testing Improve curie validation Nov 8, 2023
@cmungall
Copy link
Member

You you still working on this PR @Silvanoc ?

@Silvanoc
Copy link
Contributor Author

You you still working on this PR @Silvanoc ?

Sorry, I completely forgot this PR. Ich had a private discussion on Slack with @sierra-moxon, where she was raising her concern about the convenience of the changes implying that something like 17 would become an accepted CURIE (see the tests).

I'm quoting here her concern and my answer:

By @sierra-moxon:

Hi again - I see you opened another draft PR on this -- and are currently validating that 17 is a valid curie? I don't think that is a valid curie (I don't claim to be an expert here, but if I had data that used 17 as an identifier, I would want a validation exception generated).

By @Silvanoc:

If I've read the spec right, 17 is a valid CURIE as abc. If LinkML should only support a subset of the spec-conform CURIEs, then it probably should be specified within the project. My feeling is that you expect at least the colon : and possibly also the prefix. We can of course have validation rules for the LinkML CURIE subset. But as I've said, we should first have a clear specification. We could have something like "LinkML CURIEs are all valid CURIEs according the W3C specification providing both a prefix and the separation colon".

After that I simply forgot about it.

@cmungall what's your opinion about it? Could you formulate it with comments on the tests? Something like "I wouldn't expect this to be accepted" or otherwise.

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.

2 participants