Skip to content

Use schema, inputSchema, outputSchema from updated TD spec. Add support for TD links. #94

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

Merged
merged 5 commits into from
Feb 26, 2018

Conversation

zolkis
Copy link
Contributor

@zolkis zolkis commented Feb 16, 2018

Following w3c/wot-thing-description#95
Fixes #94 and #89.
Signed-off-by: Zoltan Kis [email protected]

index.html Outdated
@@ -854,13 +854,13 @@ <h2>Examples</h2>
thing.addProperty({
name: "temperature",
value: "0",
type: "number",
schema: "number",
Copy link
Contributor

@mkovatsc mkovatsc Feb 16, 2018

Choose a reason for hiding this comment

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

must be schema: '{ "type": "number" }',

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Thanks! Fixed.

index.html Outdated
writable: false
// use default values for the rest
});
thing.addEvent({
name: "onchange",
dataDescription: "number"
schema: "number"
Copy link
Contributor

Choose a reason for hiding this comment

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

must be schema: '{ "type": "number" }'

@mkovatsc
Copy link
Contributor

The examples must be changed from schema: "number" to schema: '{ "type": "number" }'.

@zolkis zolkis changed the title Use schema, inputSchema, outputSchema from updated TD spec Use schema, inputSchema, outputSchema from updated TD spec. Add support for TD links. Feb 16, 2018
@danielpeintner
Copy link
Contributor

I am fine with the type->schema and inputDataDescription->inputSchema or outputDataDescription->outputSchema changes.

Having said that, the getLinks() updates are pretty new (not even sure if they make it into the TD for Prague) and I would rather split this PR into two and postpone the getLinks PR at the moment.

@zolkis
Copy link
Contributor Author

zolkis commented Feb 19, 2018

Based on the discussions on today's calls, I have done the following changes:

  • removed ConsumedThing.name and setName()
  • moved getLinks() to a new section where experimental extensions are listed for informative purposes. For the sake of symmetry, I have included there getProperties(), getActions() and getEvents(), reusing the structure definitions in ExposedThing. None of these are mandatory for the next F2F.
    Please check if this is OK.

… section. Add getProperties(), getActions(), getEvents().

Signed-off-by: Zoltan Kis <[email protected]>
@danielpeintner
Copy link
Contributor

See my two comments below..

  1. setName(DOMString name) was removed again (which I think was a fault to be added in the first place). By removing setName also the readonly name was removed.
    I am fine with this change (given that the name can be retrieved by getThingDescription()). IF we do so we also need to change some examples where thing.name is used.
    Having said that, I am also fine with keeping the name attribute at the moment because it was very handy. @knimura what do you think?
    https://rawgit.com/zolkis/wot-scripting-api/a38a250800f3945f656bc8982daf2a3283e83775/index.html#examples

  2. I also noticed that ThingProperty has changed to ThingPropertyInit etc (similarly ThingAction and ThingEvent).
    I am fine with name changes (these were related to the new Experimental section) but from the developer point of view we should not keep moving till to Prague PlugFest. All these little changes are very difficult to keep track of and may cause issues.

In general PR looks good to go.

@zolkis
Copy link
Contributor Author

zolkis commented Feb 20, 2018

setName(DOMString name) was removed again (which I think was a fault to be added in the first place). By removing setName also the readonly name was removed.

This was agreed in the scripting call.

I also noticed that ThingProperty has changed to ThingPropertyInit etc (similarly ThingAction and ThingEvent).

Actually the other way around, ThingPropertyInit was changed to ThingProperty (and similarly, Init was removed as suffix) in order to support the introspection methods in the experimental section.

@knimura
Copy link
Contributor

knimura commented Feb 20, 2018

I'm ok to removing the SetName(). We can add again if TD define the corresponding spec.

@zolkis
Copy link
Contributor Author

zolkis commented Feb 20, 2018

I am also fine with keeping the name attribute at the moment because it was very handy.

Yes, it was handy, but it should be clear whether is it a user settable name independent of the TD, or is it mandatory the TD exposes a name property and that is exposed directly on the ConsumedThing object? If so, it this a general practice with Thing properties, or an exception?

I would not mind exposing Thing Properties directly on the ConsumedThing object, e.g. thing.name, thing.temperature if the TD defines them so. Introspection methods would get the full definitions anyway.
Similarly, to be consistent, we could expose actions as functions and Thing events as JS events, but this has been questioned.

@zolkis
Copy link
Contributor Author

zolkis commented Feb 20, 2018

I would not mind exposing Thing Properties directly on the ConsumedThing object

There are some issues with this:

  • implementations currently can invoke async functions for fetching property values
  • the consume() method is currently synchronous.

So, either we make consume() asynchronous, and it would fetch all property values at construction time, or (better), add a fetch() or synchronize() method to ConsumedThing, which would update properties (only for read). This would mean to make it easier to read the last fetched sample of a property value in a simpler way. However, it introduces a lot of complications.

Examples previously using thing.name should use thing.readProperty('name').then(...); - a lot more complex as well. Using await will make that code simpler, though.

So for the moment I suggest removing name and not exposing Thing properties on the ConsumedThing object.

@zolkis
Copy link
Contributor Author

zolkis commented Feb 20, 2018

I have fixed the examples containing thing.name, and added and editors note about discovery constraints being experimental feature.

@danielpeintner
Copy link
Contributor

I have fixed the examples containing thing.name, and added and editors note about discovery constraints being experimental feature.

I did see your fix by using thing.readProperty('name') which makes an assumption I don't think is true.
The assumption is that the name of a thing is also a property. This might be the case but surely does not hold in all cases.

I think the fix should be something like the following

JSON.parse(thing.getThingDescription()).name

Once we (and if we) have ways to retrieve metadata from the thing there might be different ways to get this information.

@mkovatsc
Copy link
Contributor

I would not mind exposing Thing Properties directly on the ConsumedThing object, e.g. thing.name, thing.temperature if the TD defines them so.

There is a lot of mixup here and the follow-up comment.

We use capitalized "Property" for the Interaction Pattern, which requires sending a message (usually a GET request) to the Thing. These are different from JS/JSON properties.

.name would come from the rather static TD metadata, where no additional message/GET is needed. There will never be a readProperty("name") for this metadata field.

.temperature is a Property Interaction. It needs a GET request to fetch. This is asynchronous, so JS property access is not a good API (as you said).

So: metadata attributes and readible Properties are different. Attributes (lower case properties) on ConsumedThing would expose metadata, which is also contained in what getDescription() returns. It should be clear what attributes from the metadata are accessible and which not.

I aim at having better introspection based on the simplified TD coming after Prague. It wpuld be similar to what was shown by Mozilla, e.g.,

let temp = await myThing.properties.temperature.read();

myThing.properties.setpoint.write(temp+5);

@zolkis
Copy link
Contributor Author

zolkis commented Feb 20, 2018

.name would come from the rather static TD metadata

So should the scripting API expose what you call the "TD metadata" (if TD wasn't metadata to start with)?
Namely, should be there a .name, .base, and .security properties on a Thing? But wait, .interaction is similar "TD metadata" as .name, only that it gets special treatment because the values involve network operations. Wouldn't that logically imply (by induction) that e.g. thing.interaction.myTemp (name example taken from here could be also exposed?

.name would come from the rather static TD metadata

Also, is the .name user settable?

let temp = await myThing.properties.temperature.read();
myThing.properties.setpoint.write(temp+5);

+1. By this we would reach the level of OCF APIs at least :).

Signed-off-by: Zoltan Kis <[email protected]>
@zolkis
Copy link
Contributor Author

zolkis commented Feb 26, 2018

Discussed and approved on the Scripting call today.

@zolkis zolkis merged commit 9249005 into w3c:master Feb 26, 2018
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