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 Expansion handling of type maps and @none #264

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

Issue with Expansion handling of type maps and @none #264

kasei opened this issue Dec 19, 2019 · 5 comments

Comments

@kasei
Copy link

@kasei kasei commented Dec 19, 2019

Expansion algorithm step 13.8.3.7.5:

Otherwise, if container mapping includes @type initialize types to the concatenation of expanded index with any existing values of @type in item. If expanded index is @none, do not concatenate expanded index to types. Add the key-value pair (@type-types) to item.

Based on test tm012, I don't think this key-value pair is meant to be added to item in the case that expanded index is @none. If that is the case, this text can likely be simplified, as the "If expanded index is @none" condition will apply to the entire body of this step.

Also, the word choice of "concatenation" is confusing here. By context, I believe that what is being described is an array, types, that should possibly contain expanded index, and definitely contain any existing values of @type in item. Is there a way to improve this language to not risk confusion with string concatenation?

@gkellogg

This comment has been minimized.

Copy link
Member

@gkellogg gkellogg commented Dec 20, 2019

It probably should have used wording symmetric to the previous, where a check for expanded index being @none predicates the further steps.

The following should have the intended meaning:

Otherwise, if container mapping includes @type and expanded index is not @none, initialize types to the a new array consisting of expanded index followed by any existing valuers of @type in item. Add the key-value pair (@type-types) to item.

Note that we also use "concatenation" in 13.8.3.7.2 (for @index), but I believe that concatenation is a well defined concept on arrays, so rewording simply to replace this would be gratuitous.

@gkellogg gkellogg added the wr:open label Dec 20, 2019
@kasei

This comment has been minimized.

Copy link
Author

@kasei kasei commented Dec 20, 2019

@gkellogg I think concatenation is well-defined when both operands are arrays. But in this case, it's an array and a string, right? (Which in the JS case would be push, but might also be called append...)

@kasei

This comment has been minimized.

Copy link
Author

@kasei kasei commented Dec 20, 2019

Agree with your new proposed text; it is much clearer.

gkellogg added a commit that referenced this issue Dec 23, 2019
For #264.
@gkellogg

This comment has been minimized.

Copy link
Member

@gkellogg gkellogg commented Dec 23, 2019

@kasei this is updated in PR #274. Please indicate if this satisfies your issue.

@gkellogg gkellogg added this to Editorial work complete in JSON-LD Management Dec 23, 2019
@kasei

This comment has been minimized.

Copy link
Author

@kasei kasei commented Dec 24, 2019

@gkellogg yes, looks good. Thanks.

gkellogg added a commit that referenced this issue Dec 24, 2019
@gkellogg gkellogg closed this Jan 10, 2020
@gkellogg gkellogg removed this from Editorial work complete in JSON-LD Management Jan 10, 2020
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
2 participants
You can’t perform that action at this time.