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

Issue with @container arrays #242

Closed
kasei opened this issue Dec 11, 2019 · 8 comments
Closed

Issue with @container arrays #242

kasei opened this issue Dec 11, 2019 · 8 comments

Comments

@kasei
Copy link

@kasei kasei commented Dec 11, 2019

Test t0080 uses a @container array value:

"@container": ["@graph", "@set"]

The changelog seems to suggest that this is OK:

The value for @container in an expanded term definition can also be an array containing any appropriate container keyword along with @set (other than @list). This allows a way to ensure that such entry values will always be expressed in array form.

However, Create Term Definition step 21.1 is not written in a way that supports this combination of container values:

Initialize container to the value associated with the @container entry, which MUST be either @graph, @id, @index, @language, @list, @set, @type, or an array containing exactly any one of those keywords, an array containing @graph and either @id or @index optionally including @set, or an array containing a combination of @set and any of @index, @id, @type, @language in any order . Otherwise, an invalid container mapping has been detected and processing is aborted.

I read this as indicating that ["@graph", "@set"] is an invalid container mapping, and processing will be aborted. Is that correct? Can this text and/or the test suite be updated to clarify this?

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

This comment has been minimized.

Copy link
Member

@gkellogg gkellogg commented Dec 12, 2019

No, it is permissible to use ["@graph", "@set"], and there are steps in both Compaction and IRI Compaction that take advantage of this. @graph should be added to the list of keywords for which @set can be used.

@pchampin

This comment has been minimized.

Copy link
Contributor

@pchampin pchampin commented Dec 13, 2019

Actually the part that says

containing @graph and either @id or @index optionally including @set

does cover that case...

@kasei

This comment has been minimized.

Copy link
Author

@kasei kasei commented Dec 13, 2019

@pchampin how does that cover this case when the container does not contain either @id or @index?

@pchampin

This comment has been minimized.

Copy link
Contributor

@pchampin pchampin commented Dec 13, 2019

oh, you are right :-/ sorry

@iherman

This comment has been minimized.

Copy link
Member

@iherman iherman commented Dec 13, 2019

This issue was discussed in a meeting.

  • RESOLVED: Update api Create Term Definition step to allow the set of combinations from syntax 9.15.1
View the transcript 4.4. @graph, @set
Rob Sanderson: #242
Rob Sanderson: "@container": ["@graph", "@set"] is used in a test, but the text of the algorithm implies that it is illegal
Gregg Kellogg: this is an ommision in the algorithm in the list of things allowed with @graph
… the normative section of the syntax does allow this combination
Gregg Kellogg: http://localhost:8000/?specStatus=CR&publishDate=2019-12-12#expanded-term-definition
Pierre-Antoine Champin: https://www.w3.org/TR/json-ld11/#expanded-term-definition
Rob Sanderson: > If the expanded term definition contains the @container keyword, its value MUST be either @list, @set, @language, @index, @id, @graph, @type, or be null or an array containing exactly any one of those keywords, or a combination of @set and any of @index, @id, @graph, @type, @language in any order
Rob Sanderson: so the API and Syntax documents are out of sync
Gregg Kellogg: there was examples specifically added about it
Proposed resolution: Update api Create Term Definition step to allow the set of combinations from syntax 9.15.1 (Rob Sanderson)
Gregg Kellogg: +1
Rob Sanderson: +1
David I. Lehn: +1
Ivan Herman: 0
Tim Cole: +1
Harold Solbrig: +1
Benjamin Young: +1
Pierre-Antoine Champin: +1
Resolution #5: Update api Create Term Definition step to allow the set of combinations from syntax 9.15.1
gkellogg added a commit that referenced this issue Dec 17, 2019
@gkellogg gkellogg moved this from Discuss-Call to Editorial work complete in JSON-LD Management Dec 17, 2019
@gkellogg

This comment has been minimized.

Copy link
Member

@gkellogg gkellogg commented Dec 18, 2019

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

gkellogg added a commit that referenced this issue Dec 18, 2019
gkellogg added a commit that referenced this issue 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
4 participants
You can’t perform that action at this time.