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

Possible problem with Create Term Definition algorithm on compact IRI terms #245

Closed
kasei opened this issue Dec 12, 2019 · 5 comments
Closed

Comments

@kasei
Copy link

@kasei kasei commented Dec 12, 2019

Based on test t0025, I believe that Create Term Definition, step 16.5 is wrong.

Processing any term at this point with the syntactic form of a compact IRI will fail with a 'cyclic IRI mapping' error after calling IRI expansion, which then calls (in step 3) Create Term Definition again.

I notice that setting defined[term] to true in 16.5 immediately before calling IRI expansion resolves the issue for this particular test, but I do not know what the implications are for possible impact elsewhere in the spec and/or testsuite.

@azaroth42 azaroth42 added this to Discuss-Call in JSON-LD Management Dec 12, 2019
@gkellogg

This comment has been minimized.

Copy link
Member

@gkellogg gkellogg commented Dec 12, 2019

Looking at my code, it doesn't pass either local context or defined to Create Term Definition and passes the tests, but can potentially lead to a case where term hasn't yet been defined, so your solution would seem to be correct.

@iherman

This comment has been minimized.

Copy link
Member

@iherman iherman commented Dec 13, 2019

This issue was discussed in a meeting.

  • RESOLVED: Add to Create Term Definition 16.5 that defined should be set to True, and that it should be resolved as vocabulary relative by adding the appropriate parameter
View the transcript Create Term Definition 16.5 should set defined?
Rob Sanderson: #245
Gregg Kellogg: when a term has the form of a CURIE, it is possible that the prefix has not yet been defined
… so we recursively call “expand IRI”;
… we keep track of defined terms, this should be updated accordingly
… also, we should have that it should be resolved against the vocabulary
Proposed resolution: Add to Create Term Definition 16.5 that defined should be set to True, and that it should be resolved as vocabulary relative by adding the appropriate parameter (Rob Sanderson)
Gregg Kellogg: +1
Rob Sanderson: +1
Benjamin Young: +1
Pierre-Antoine Champin: +1
Ivan Herman: +1
Harold Solbrig: +1
Tim Cole: +1
Resolution #7: Add to Create Term Definition 16.5 that defined should be set to True, and that it should be resolved as vocabulary relative by adding the appropriate parameter
@gkellogg

This comment has been minimized.

Copy link
Member

@gkellogg gkellogg commented Dec 18, 2019

@kasei, please let us know if changes in #251 satisfactorily address your concern.

@gkellogg gkellogg moved this from Discuss-Call to Editorial work complete in JSON-LD Management Dec 18, 2019
@kasei

This comment has been minimized.

Copy link
Author

@kasei kasei commented Dec 18, 2019

Looks good, thanks.

@iherman

This comment has been minimized.

Copy link
Member

@iherman iherman commented Dec 20, 2019

This issue was discussed in a meeting.

  • RESOLVED: Close completed editorial issues API#242, #243, #244, #245, #246
View the transcript Close editorial issues
Proposed resolution: Close completed editorial issues API#242, #243, #244, #245, #246 (Rob Sanderson)
Ruben Taelman: +1
Gregg Kellogg: +1
Ivan Herman: +1
Pierre-Antoine Champin: +1
Jeff Mixter: +1
Tim Cole: +1
Rob Sanderson: +1
Adam Soroka: +1
Resolution #9: Close completed editorial issues API#242, #243, #244, #245, #246
@iherman iherman closed this Dec 20, 2019
@gkellogg gkellogg removed this from Editorial work complete in JSON-LD Management Dec 23, 2019
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
3 participants
You can’t perform that action at this time.