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 subscribeallevents and unsubscribeallevents operations - closes #1082 #1191

Merged
merged 3 commits into from
Jul 14, 2021

Conversation

benfrancis
Copy link
Member

@benfrancis benfrancis commented Jul 13, 2021

As discussed in #1082.

Note that these new ops are needed for the Directory Service API in the WoT Discovery specification (see w3c/wot-discovery#159), and for making WebThings Gateway W3C compliant (see WebThingsIO/gateway#2802 (comment)).

Also fixed a typo on observeallproperties in the ontology.


Preview | Diff

Copy link
Member

@relu91 relu91 left a comment

Choose a reason for hiding this comment

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

PR looks good. Two comments:

  • The changes somehow touched also unexpected files (like visualizaton/tm.svg) and the preview does not show the TD model graph anymore. Is it a bug in the preview renderer?
  • The other operation types are described with a short sentence in wot-architecture. In the very same document an editor's note states that Other specifications may define further well-known operation types that are valid for their respective document format or form serialization should we spend few words to explain subscribeallevent, unsubscribeallevent etc. in the TD document? It might be beneficial to have also examples about this new operations, like the one with SSE.

@benfrancis
Copy link
Member Author

benfrancis commented Jul 13, 2021

Thanks for the review @relu91.

The changes somehow touched also unexpected files (like visualizaton/tm.svg) and the preview does not show the TD model graph anymore. Is it a bug in the preview renderer?

I found it quite hard to figure out which files are meant to be edited manually and which are meant to be generated by render.sh.

I used 5ac3b21 as a guide to figure out which files needed to change, then tried to figure out from the comments in the rendering script which files to edit vs. generate. The files I edited manually were:

  1. context/hypermedia-context.jsonld
  2. validation/td-validation.ttl
  3. validation/ext-td-json-schema-validation.json
  4. validation/td-json-schema-validation.json
  5. ontology/td.ttl
  6. index.template.html

All the other changes were automatically generated when running npm run render. Please advise whether I should do any of that differently or exclude any files from the diff.

The other operation types are described with a short sentence in wot-architecture. In the very same document an editor's note states that Other specifications may define further well-known operation types that are valid for their respective document format or form serialization should we spend few words to explain subscribeallevent, unsubscribeallevent etc. in the TD document? It might be beneficial to have also examples about this new operations, like the one with SSE.

I think we may have discussed this before but in my view Table 1 from 7.6.2 Forms in the WoT Architecture specification should be moved to the Thing Description specification since it's not immediately obvious from the Thing Description specification where to find those definitions or even that they exist.

I'm happy to add a description of subscribeallevents and unsubscribeallevents somewhere in the Thing Description specification, but it would probably be better to keep them all in one place?

Edit: I think observeallproperties and unobserveallproperties are also currently missing from that table?

@relu91
Copy link
Member

relu91 commented Jul 13, 2021

Thanks for the review @relu91.

The changes somehow touched also unexpected files (like visualizaton/tm.svg) and the preview does not show the TD model graph anymore. Is it a bug in the preview renderer?

I found it quite hard to figure out which files are meant to be edited manually and which are meant to be generated by render.sh.

I used 5ac3b21 as a guide to figure out which files needed to change, then tried to figure out from the comments in the rendering script which files to edit vs. generate. The files I edited manually were:

  1. context/hypermedia-context.jsonld
  2. validation/td-validation.ttl
  3. validation/ext-td-json-schema-validation.json
  4. validation/td-json-schema-validation.json
  5. ontology/td.ttl
  6. index.template.html

All the other changes were automatically generated when running npm run render. Please advise whether I should do any of that differently or exclude any files from the diff.

Yeah, I feel you. it would be better to have some guidance in a readme file, plus some live PR validation using a GitHub action. To me, the file list seems correct. Maybe some npm install artifacts? (i.e., it installed a newer version of the graph rendering tool which caused the changes?)

The other operation types are described with a short sentence in wot-architecture. In the very same document an editor's note states that Other specifications may define further well-known operation types that are valid for their respective document format or form serialization should we spend few words to explain subscribeallevent, unsubscribeallevent etc. in the TD document? It might be beneficial to have also examples about this new operations, like the one with SSE.

I think we may have discussed this before but in my view Table 1 from 7.6.2 Forms in the WoT Architecture specification should be moved to the Thing Description specification since it's not immediately obvious from the Thing Description specification where to find those definitions or even that they exist.

I remember that we mentioned this issue before, maybe in profile. The current direction seems to have most of the concepts in the architecture document and then refer back to it from the TD spec. I don't have a strong opinion about this as long as all the operations are correctly defined/explained 😄 . It might make sense to do this in another PR then.

I'm happy to add a description of subscribeallevents and unsubscribeallevents somewhere in the Thing Description specification, but it would probably be better to keep them all in one place?

Yes I would prefer to have them all in one place 👍🏻

Edit: I think observeallproperties and unobserveallproperties are also currently missing from that table?

yes, they are missing too!

ontology/td.ttl Outdated Show resolved Hide resolved
@benfrancis
Copy link
Member Author

@relu91 wrote:

Maybe some npm install artifacts? (i.e., it installed a newer version of the graph rendering tool which caused the changes?)

Could be, yes. I'm not sure how to fix that. Shall I remove some of the generated files from the diff?

Yes I would prefer to have them all in one place 👍🏻

OK, let's create a follow-up PR for Architecture to add descriptions for the missing options, since that's the current home of the table.

@benfrancis
Copy link
Member Author

benfrancis commented Jul 14, 2021

@relu91 Which generated files do you think should be removed? All the SVG files?

@benfrancis
Copy link
Member Author

OK, I reverted the SVG changes.

@sebastiankb
Copy link
Contributor

from today's TD call, group decided to merge this PR

@sebastiankb sebastiankb merged commit e8d15f0 into w3c:main Jul 14, 2021
takuki added a commit to takuki/wot-thing-description that referenced this pull request Jan 29, 2022
egekorkan pushed a commit to egekorkan/wot-thing-description that referenced this pull request Feb 3, 2022
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.

4 participants