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

Add alternative payload format (2) #213

Merged
merged 5 commits into from
Jul 26, 2021
Merged

Add alternative payload format (2) #213

merged 5 commits into from
Jul 26, 2021

Conversation

wiresio
Copy link
Member

@wiresio wiresio commented Jul 18, 2021

Rework of #168

resolves #16


Preview | Diff

@wiresio wiresio changed the title Add alternative payload Add alternative payload format (2) Jul 18, 2021
@farshidtz
Copy link
Member

Thanks. I think the content is fine, but it needs some more work:

  • The example is not correct. totalItems is 10 and all those 10 are listed: so no next pages. There is still a nextLink with offset 1. Should there be a nextLink when there are no remaining pages? If yes, the offset should be 10. I think a better fix would be to set total to 12 and nextLink to /things?offset=10&limit=10 to be consistent with the other pagination examples (array format).
  • The API endpoint should be /things, not /td
  • Missing API spec for the new interaction. I think addition of the new uriVariable (?format=array|hydraCollection) on the existing pagination form would suffice.

Some editorial issues:

  • The new paragraphs should be added at the end of Listing (before errors) as this is an alternative to pure array. The existing paragraph starting with "Serializing and returning the full list of TDs may be burdensome to servers. As such, servers should serialize incrementally and utilize protocol-specific mechanisms to respond in chunks...." should be moved to after the hydra collection.
  • Missing hyrda references
  • Code as italic text instead of using <code> tag or backquotes

@wiresio
Copy link
Member Author

wiresio commented Jul 19, 2021

Thanks for the feedback @farshidtz ! I applied your comments and will need some help with:

  • Add references
  • "code" instead of "i"

Let's resolve it in today's meeting.

index.html Outdated
in <a href="https://www.hydra-cg.com/spec/latest/core/#advanced-concepts">
Hydra Advanced Concepts</a>. More concretely the
<a href="https://www.hydra-cg.com/spec/latest/core/#example-20-a-hydra-partialcollectionview-splits-a-collection-into-multiple-views">
Partial Collection View</a> using the <i>members</i> field to accomodate the array
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

"members" -> "member" ?

@mmccool
Copy link
Contributor

mmccool commented Jul 19, 2021

Needs some refinement. In particular, as discussed, it's troublesome to depend normatively on a CG document, and it's also a problem to just define an extension point (with hydra collections as examples) since that creates interop problems. We need a built-in normative way to do this, but can't just adopt the hydra ontology directly. So it was suggested that we resurrect the design we had discussed in #16 and which can be seen in the context history, see e.g. de02ff0#diff-94098f295e6c8f89beca5d1221bbaefaf5c410e17b1a6be3d5df9051b205f72f

@farshidtz
Copy link
Member

farshidtz commented Jul 26, 2021

Since we are not strictly following hydra anymore, why don't we use plural items instead of the single member?

https://github.com/wiresio/wot-discovery/blob/66284902a8892fdf9764900f75c51c21fe958f36/index.html#L1419-L1423

"totalItems": 12,
"member": [
	{ TD },
	9 other TDs...
],

to

"totalItems": 12,
"items": [
	{ TD },
	9 other TDs...
],

The following maps better:
totalItems -> items
totalMembers -> memberS

or simply:
total + items or
total + members

@wiresio
Copy link
Member Author

wiresio commented Jul 26, 2021

Thanks for the comment!
Actually I'd like:
total + members
We should also check the context - I'm not sure whether I put it right.

},
"total": "discovery:contains",
"members": [
"@graph"
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This perhaps should be td:Thing included from the td context, since in the intent is to return TDs (formatted as TDs) and not general RDF graphs.

@AndreaCimminoArriaga
Copy link
Contributor

I have to review in detail the new context, but maybe it would be better to have a separate context with the pagination terms, and the range of the term members should point to td:Thing or any of the types defined in the discovery context. In this way this second context is totally optional and would point to the main one

@mmccool mmccool merged commit a64f17c into w3c:main Jul 26, 2021
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Handle huge set of Thing Descriptions (pagination, streaming, etc.)
4 participants