-
Notifications
You must be signed in to change notification settings - Fork 203
IRI fixes #333
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
Conversation
Resolve relative `@vocab` using expandIri with both vocab and base relative resolution. A relative `@vocab` is only invalid in 1.0.
… a term that can be used as a prefix.
return mapping['@id'] + suffix; | ||
} | ||
|
||
// already absolute IRI | ||
return value; | ||
if(_isAbsoluteIri(value)) { |
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.
Why is this extra check needed now? It still appears to be within the positive check for a colon in value
.
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.
Having a colon isn’t sufficient to make it an absolute (or compact) IRI. This final test ensure that only an absolute IRI will be returned, otherwise, if falls through.
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.
Ok.
@@ -66,7 +66,7 @@ api.prependBase = (base, iri) => { | |||
return iri; | |||
} | |||
// already an absolute IRI | |||
if(iri.indexOf(':') !== -1) { | |||
if(api.isAbsolute(iri)) { |
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.
@davidlehn -- it would be good to understand the performance impact on things like this.
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.
We do have benchmarking code. I'm not sure what the tests would be.
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.
While a regex comparison is more compute intensive than an indexOf, they should be pretty similar. In any case, it's incorrect to just look for a ':' as a test for if an IRI is absolute.
Co-Authored-By: Dave Longley <[email protected]>
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.
LGTM modulo @davidlehn's typo fix on my bad typo fix.
return mapping['@id'] + suffix; | ||
} | ||
|
||
// already absolute IRI | ||
return value; | ||
if(_isAbsoluteIri(value)) { |
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.
Ok.
Co-Authored-By: David I. Lehn <[email protected]>
@vocab
can be relative or a Compact IRI in 1.1, resolved against either a previous@vocab
,@base
or document base.@prefix
: false, or expanded term definitions to construct compact IRIs.