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 test t0004 #310

Closed
kasei opened this issue Jan 8, 2020 · 5 comments
Closed

Issue with expansion test t0004 #310

kasei opened this issue Jan 8, 2020 · 5 comments

Comments

@kasei
Copy link

@kasei kasei commented Jan 8, 2020

Expansion test t0004 seems to expect the value from the recursive expansion call in 13.4.11.2 to be an array, but it's coming back (from Value Expansion) as a map. This seems related to #299 which would have ensured that the value coming back form the recursive call was an array.

If t0004 represents the expected behavior, does 13.4.11.2 need to duplicate some of the post-processing that #299 moved into the JsonLdProcessor API?

@gkellogg gkellogg added this to Editorial Work in JSON-LD Management Jan 10, 2020
@gkellogg gkellogg added the wr:open label Jan 10, 2020
@gkellogg

This comment has been minimized.

Copy link
Member

@gkellogg gkellogg commented Jan 10, 2020

The result of 13.4.11.2 is, indeed, a map of the form {"@list": [...]}, but it will have been invoked from 13.14.2 would have added it to an array value of the associated property.

It is only at the outer level that the API algorithm comes in to place to make sure that the value of there outermost map gets wrapped in an array. Values of properties always get represented as arrays due to 13.14.2.

@kasei

This comment has been minimized.

Copy link
Author

@kasei kasei commented Jan 13, 2020

I don't see how 13.14.2 helps here. I believe in this case, the value it will be appending to a list is:

{
	"@list": {
		"@value": "one item"
	}
}

The list being appended to is result["http://example.org/property"]. So we've got this:

                'http://example.org/property' => [
                                                   {
                                                     '@list' => {
                                                                  '@value' => 'one item'
                                                                }
                                                   }
                                                 ],

But the test is expecting this:

                'http://example.org/property' => [
                                                   {
                                                     '@list' => [
                                                                  {
                                                                    '@value' => 'one item'
                                                                  }
                                                                ]
                                                   }
                                                 ],

I think the list introduced in 13.14.2 is operating at the wrong level. It needs to happen somewhere between the value being set to expanded property in 13.4.11.2 and that value being used to set the result entry @list-expanded property in 13.4.16.

@gkellogg

This comment has been minimized.

Copy link
Member

@gkellogg gkellogg commented Jan 14, 2020

It looks like 13.4.11.2 should add the words "ensuring that the result is an array." as is done in 13.4.6.

@gkellogg

This comment has been minimized.

Copy link
Member

@gkellogg gkellogg commented Jan 14, 2020

I actually had a FIXME in my code that this needed to be added! Let me know if that does it.

@kasei

This comment has been minimized.

Copy link
Author

@kasei kasei commented Jan 14, 2020

@gkellogg the proposed change too 13.4.11.2 is in fact what I had come up with in attempting to move past this issue, and the wording now matches 13.4.6.2. Looks good.

gkellogg added a commit that referenced this issue Jan 15, 2020
@gkellogg gkellogg closed this Jan 15, 2020
@gkellogg gkellogg removed this from Editorial Work in JSON-LD Management Jan 15, 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.