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 order of processing nesting keys in Expansion #295

Closed
kasei opened this issue Jan 1, 2020 · 6 comments
Closed

Issue with order of processing nesting keys in Expansion #295

kasei opened this issue Jan 1, 2020 · 6 comments

Comments

@kasei
Copy link
Contributor

kasei commented Jan 1, 2020

Test tn004 seems to rely on nesting keys being processed in lexicographic order. However, Expansion step 14 does not specify any order on the iteration:

For each key nesting-key in nests

I believe this should instead be:

For each key nesting-key in nests ordered lexicographically

@gkellogg
Copy link
Member

gkellogg commented Jan 2, 2020

Could be optionally ordered as other loops do, but not necessary for interop, given instructions on comparing results.

@kasei
Copy link
Contributor Author

kasei commented Jan 2, 2020

@gkellogg doesn't tn004 depend on the ordering here?

@gkellogg
Copy link
Member

gkellogg commented Jan 2, 2020

If you see the note on JSON-LD Object comparison, it says that arrays are compared without regard to order. Of course it also says that if the ordered flag is true, you can use simple object comparison, so adding the ordering text would be predicated on the ordered option.

@gkellogg
Copy link
Member

gkellogg commented Jan 2, 2020

@kasei I added words to order nesting-key if ordered is true.

As always, please respond if this satisfies your concern.

@kasei
Copy link
Contributor Author

kasei commented Jan 2, 2020

#297 looks reasonable, thanks.

I had missed the wording about test comparisons and instead got caught up by the title of the test ("Test tn004 Appends nested values from all @nest aliases in term order"; emphasis added). Am I misunderstanding the implication of the title? Or should it be updated?

@gkellogg
Copy link
Member

gkellogg commented Jan 3, 2020

Yes, the "in term order" should be dropped from that test entry, as it's inconsistent with the optional ordering of the algorithm. Ordering can be very expensive, and prohibits many streaming implementations.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

No branches or pull requests

2 participants