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 error in Expansion Algorithm handling of nesting values (13.15) #262

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

Possible error in Expansion Algorithm handling of nesting values (13.15) #262

kasei opened this issue Dec 19, 2019 · 10 comments

Comments

@kasei
Copy link

@kasei kasei commented Dec 19, 2019

I believe that the Expansion Algorithm's handling of nesting values in step 13.15 appears at the wrong nesting level of the algorithm, and that it should really occur after step 13 as a new step 14 (renumbering all subsequent steps). I suspect this is the case because:

  • Having this step within the loop of step 13 makes it dependent on the ordering of keys that are looped over
  • Having this step within the loop of step 13 can end up applying the same nesting values to the result over and over again (with an upper limit of the number of the number of non-keyword keys handled by the 13 loop)
  • Doing this allows test tn006 to pass, while following the spec does not

If this is true, the language in the (current) 13.15.2.2 about "recursively" applying step 13 probably needs to be changed, as it will no longer be recursive.

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

This comment has been minimized.

Copy link
Member

@gkellogg gkellogg commented Dec 20, 2019

Yes, it should be outside the loop as step 14.

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

This comment has been minimized.

Copy link
Member

@iherman iherman commented Dec 20, 2019

This issue was discussed in a meeting.

  • RESOLVED: Accept API #262 and move 13.15 to new step 14, renumber following steps (and can close when complete)
View the transcript Nest handling at wrong level of indent
Rob Sanderson: #262
Rob Sanderson: There were a couple of expansion issues that would cause it to blow up. The problem is that nested entries in the context, would recursively require processing of that node.
… We figured out that it was just an indentation issue in the algorithm, which fixes the problem.
Proposed resolution: Accept API #262 and move 13.15 to new step 14, renumber following steps (and can close when complete) (Rob Sanderson)
Gregg Kellogg: +1
Rob Sanderson: +1
Jeff Mixter: +1
Ruben Taelman: +1
Tim Cole: +1
Ivan Herman: +1
Adam Soroka: +1
Pierre-Antoine Champin: +1
Resolution #6: Accept API #262 and move 13.15 to new step 14, renumber following steps (and can close when complete)
@azaroth42 azaroth42 moved this from Discuss-Call to Editorial Work in JSON-LD Management Dec 20, 2019
@kasei

This comment has been minimized.

Copy link
Author

@kasei kasei commented Dec 20, 2019

I see that this was resolved this morning, but I wanted to let you know that I'm now not certain that turning 13.15 into 14 is going to resolve the issue entirely. I believe test tn005 highlights a case that complicates this, and which will fail with 13.15 moved to 14. I'm not yet certain how to handle the recursion of @nest values in this test, but something clearly has to be updated in 13.15.

@gkellogg

This comment has been minimized.

Copy link
Member

@gkellogg gkellogg commented Dec 21, 2019

@kasei this is fixed in PR #272. Please let us know if this satisfies the issue.

@gkellogg gkellogg moved this from Editorial Work to Editorial work complete in JSON-LD Management Dec 21, 2019
@gkellogg

This comment has been minimized.

Copy link
Member

@gkellogg gkellogg commented Dec 21, 2019

@kasei Step 14.2.2 is to repeat step 13 until there are no more nested values, but part of what should be repeated is step 14. My implementation breaks step 13 and 14 into a method (expand_object), so perhaps changing 14.2.2 from saying "repeat step 13" to "repeat steps 13 and 14".

@kasei

This comment has been minimized.

Copy link
Author

@kasei kasei commented Dec 24, 2019

@gkellogg I agree that using "repeat steps 13 and 14" captures the intent here.

However, the text now has some very strange linguistic use of "recursively" to apply to 2 steps in a much larger algorithm, one of which is not a recursive call. I think your approach of implementing 13 and 14 as a single method is intuitive here (and likely the sensible path for many implementations), but the current language obscures this fact. I think it would be wise to improve the language or structure to either make explicit that 13 and 14 are structurally related (and recursive) or to remove the "recursive" language (perhaps in favor of "repeat" and the use of "set element to nested value" instead of "using nested value for element").

@gkellogg

This comment has been minimized.

Copy link
Member

@gkellogg gkellogg commented Dec 24, 2019

It is a recursive call, in that when invoking step 14 again, it may, in turn, use 13 and 14 with new inputs.

Of course, we could have moved all the sub-points of 13 down a level, and moved 14 back to 13.2, and just said to invoke step 13 recursively, but that seems to add a lot of structure that's not otherwise very useful.

Perhaps a note in step 14 that describes how 13 and 14 are related, and the purpose of doing such a "recursive" call is.

@kasei

This comment has been minimized.

Copy link
Author

@kasei kasei commented Dec 24, 2019

Yes, such a note might help. A number of times while implementing, I've felt that some of the structural relationships that may be completely obvious to you as both an editor and an existing implementor are not as obvious to me (and I suspect by extension to others who do not have your background with JSON-LD). I understand the resistance to making big structural changes in the algorithms at this point, but this is one of the cases where I think it might be very helpful for would-be implementors to get a more intuitive understanding of the algorithms.

Ideally I think this line of reasoning might have split 13 and 14 out into their own algorithm to make it dead-simple to understand that these things are related, recursive, and probably ought to be implemented as a stand-alone function/method. But I'm sure at CR you're past the point of being able to make a change like that.

@gkellogg

This comment has been minimized.

Copy link
Member

@gkellogg gkellogg commented Dec 24, 2019

Okay, see if the added note provides enough explanation. I'm not inclined to create a new algorithm encapsulate these steps at this point.

@kasei

This comment has been minimized.

Copy link
Author

@kasei kasei commented Dec 24, 2019

Yes, that helps, and I'm willing to accept it as resolving this issue.

@gkellogg gkellogg closed this Dec 24, 2019
@gkellogg gkellogg removed this from Editorial work complete in JSON-LD Management Dec 27, 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.