-
Notifications
You must be signed in to change notification settings - Fork 572
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
resolve issue1868, add a method to expand qname to URI #1869
Conversation
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Happy to make the suggested changes if you want.
rdflib/namespace/__init__.py
Outdated
ns = self.store.namespace(curie.split(":")[0]) | ||
if ns is not None: | ||
return URIRef(str(ns) + curie.split(":")[1]) | ||
return None |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I'm also a bit on the fence on returning None, I would just raise an error, but I would not say this is wrong, would like to hear some other opinions on the matter.
CC: @RDFLib/core-reviewers
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I prefer raising an error: if the thing asked for cannot be calculated, an error should be returned, not None which just indicates it doesn't exist. This is a calculation function, not a look up value function.
Made a PR against your branch changes logic slightly to be more compliant with CURIE spec: |
Fix handling of blank CURIE reference and references with multiple colons
…st of objects/messages.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Thanks @gjhiggins, very useful method to have.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Nice, looks good.
many thanks @gjhiggins for answering the feature request and adding this new feature. It works beautifully. |
Summary of changes
In response to issue #1868:
Seems a reasonable enough request and trivial to resolve... add NamespaceManager method
expand_qname
and tests.Checklist
the same change.
./examples
for new features.CHANGELOG.md
).so maintainers can fix minor issues and keep your PR up to date.