Update WebIDL and modify documentation to fit new "node.js-like" style #1800

t-harvey · 2018-01-28T23:39:13Z

Hi -- this is a test edit to spark discussion; please don't accept this pull request!

After a short discussion (#1759), I edited three different .md files in the documentation to get feedback.

The thrust of the changes is that I want to fix the WebIDL to make it not only syntactically correct, but also to make it match the textual description of each api.

However, since Geoff Gustafson updated Buffer.md to be more like node.js documentation, I also changed a couple of the files to match this style.

I propose making similar changes across the documentation, but I would like to limit my commitment to what I've done here -- specifically, I want to make the WebIDL match the description, but my manager would prefer that correctness of the documentation itself be left to other folks :-). For an example of this, see ble.md, where I left the description of the Characteristic API blank (seach for "TODO") (the 2nd TODO was already in the documentation).

Thanks for considering this!
Tim

be syntactically correct and to follow the new style of buffer.md. Added a new file that explains some of the differences between WebIDL and Javascript.

Some changes to a few .md files to get feedback.

grgustaf

I reviewed about 2/3 so far but will try to get back to it tomorrow.

grgustaf · 2018-01-29T21:05:38Z

docs/aio.md

+This IDL provides an overview of the interface; see below for
+documentation of specific API functions.  Click
+[here](Notes_on_WebIDL.md) for an explanation of zephyr.js' WebIDL
+conventions.


Please replace zephyr.js' with ZJS. The repo is zephyr.js for historical reasons but that's not the name of the project.

And this is not required, but I prefer to make links be noun phrases describing what you're going to find, so I'd rather see "ZJS WebIDL conventions" be what is linked, which would require some rephrasing.

Same comment on repeated instances of this text below.

grgustaf · 2018-01-29T21:08:40Z

docs/aio.md

-Use the AIOPin object returned to read values from the pin.
-
-### AIOPin.read
+Returns an AIOPin object that be used to read values from the pin.


that may be used

grgustaf · 2018-01-30T00:42:03Z

docs/aio.md


-### AIOPin.readAsync
+### pin.readAsync(ReadCallback)
+* 'ReadCallback' *callback* User-provided callback function that takes


If callback is like a type and the variable name is ReadCallback, let's call it readCallback instead to match case in other arguments. (I'm a little confused. :D)

So I'll respond here; push the explanation to the other places it applies:

I inconsistently read the Buffer documentation and just got this wrong.

I've now edited the documents to fit the following rules (which match the examples in the WebIDL spec):

Variable names are camelCase, starting with a lower-case letter.
Type names are UpperCase.

grgustaf · 2018-01-30T00:42:37Z

docs/aio.md


-Closes the AIOPin. Once it is closed, all event handlers registered will no
+Closes the AIOPin. Once it is closed, all registered event handlers will no


Nice English / voice edits - thanks.

I'm as human as the next guy, so I make these errors, too.
I'm also as OCD as the next guy, so expect more of these... :-)

grgustaf · 2018-01-30T00:45:18Z

docs/ble.md

+};
+
+callback ReadCallback = void (unsigned long offset,
+                              FulfillReadCallback fulfillreadcallback);


This is why I'm getting confused - here FulfillReadCallback is treated like a type. So maybe in my earlier comment ReadCallback was the "type" and the variable should just be callback?

As many ex-girlfriends have said to me: it's not you, it's me. :-)

The confounding inconsistency has been fixed (see above comment for more).

grgustaf · 2018-01-30T00:47:05Z

docs/ble.md

+  that will direct users to the given URL. At that URL they might be
+  able to interact with the advertising device somehow.
+
+Advertises the name of the device.


name and url

url is really the interesting bit

grgustaf · 2018-01-30T01:04:03Z

Also please fix the trailing whitespace issues here: https://travis-ci.org/intel/zephyr.js/jobs/334508966

kenchris · 2018-01-30T10:10:16Z

docs/Notes_on_WebIDL.md

+API should be considered correct -- please report discrepancies as
+soon as possible.
+
+Although both WebIDL and Javascript are aimed at web applications,


kenchris · 2018-01-30T10:12:42Z

docs/aio.md

 interface AIO {
    AIOPin open(AIOInit init);
 };

 dictionary AIOInit {
-    unsigned long pin;
+    (unsigned long or string) pin;


Many many web devs are using TypeScript today, so I see more and more web frameworks actually documenting APIs using typescript. This is also useful because you can distribute the documentation as *.ts.d files which can be used in IDE's like VSCode to do autocompletion etc.

The above would be

interface AIOInit { pin: number|string; }

@t-harvey is working on a tool to generate code based on WebIDL. So for his purposes I think he needs WebIDL specifically, or is that itself up to suggestion? Earlier I suggested maybe we should move the WebIDL itself out to a .idl file. We could also have TypeScript in a .ts.d file or both... the only question would be how well would we actually maintain them. :)

If we could turn this into some automated testing that might alert us of mismatch (because of failures) and we'd be more likely to keep it synced correctly.

We did have a existing PR that had typescript before #1572, but that was providing auto-completetions, and I haven't figure out how to import it into the ide without conflicting with other .ts.d files, so i had to close the PR, but we do have similar apis.

Geoff, I'd love to move the WebIDL to its own file and then just include it in the .md file -- but markdown doesn't seem to allow such a thing (correct me if I'm wrong). It's true that I could just have a link to a .idl file in the main page, but I tried that, and it doesn't look nearly as nice as having the WebIDL inline -- and I really like the streamlined API that WebIDL gives you in addition to the textual description; it's a lot like having a picture to reference.

Currently, I cut/paste the WebIDL out of the docs into their own .idl file and test them (which is why I missed the problem that @kenchris caught).

Yeah, that cut and pasting was part of my motivation. But yes I was thinking of just having a link to it. Hopefully by standardizing the way the WebIDL appears you can write a script to cut and paste automatically. I don't have any reason to force them out into .idl files right now, but eventually that might be best. One thing about the "Node.js" style is the functions are all listed again in the table of contents so it's a bit repetitive.

kenchris · 2018-01-30T10:15:24Z

docs/ble.md


 ```javascript
 // require returns a BLE object
 // var ble = require('ble');

-[NoInterfaceObject]
+[ReturnFromRequire]
 interface BLE: EventEmitter {


And this would be

class BLE extends EventEmitter { disconnect(address: string): void; startAdvertising(name: string, uuids: string[], url: string): void; stopAdvertising(): void; setServices(services: PrimaryService[]) void; newPrimaryService(init: PrimarySeviceInit): PrimaryService; ...

It is very JS like but has a bit of extra info, like types, readonly etc

kenchris · 2018-01-30T10:16:20Z

docs/ble.md

 callback WriteCallback = void (Buffer data, unsigned long offset,
-                               boolean withoutResponse, FulfillWriteCallback);
+                               boolean withoutResponse,
+							   FulfillWriteCallback fulfillwritecallback);


really weird indentation

Yeah, sorry; if you ever see this again, it's b/c Emacs tabs have to be converted to spaces for the Markdown to line up correctly.

kenchris · 2018-01-30T10:18:05Z

docs/buffer.md

+    octet readUInt8(optional unsigned long offset = 0);
+    short readUInt16BE(optional unsigned long offset = 0);
+    short readUInt16LE(optional unsigned long offset = 0);
+    long readUInt32BE(optional unsigned long offset = 0);


in TS: readUInt32BE(offset: number = 0): number

kenchris · 2018-01-30T10:19:34Z

docs/buffer.md

+    unsigned long copy(Buffer target, optional unsigned long targetStart = 0,
+                                      optional unsigned long sourceStart = 0,
+                                      optional unsigned long sourceEnd = this.length);
+    this fill((string or Buffer or long) value, optional long offset = 0,


You can not add "this" in an API. and same with this.length.

Could be Buffer fill(value, offset, end = -1) then you enterpret -1 to measure yourself or so

grgustaf

@t-harvey, does the WebIDL you've added/corrected work with your tool, or is that still being developed?

If we go to the trouble of making the WebIDL correct, I'd like to make sure we keep it that way by validating it with CI somehow. Could your tool or something else check for at least basic validity?

grgustaf · 2018-01-30T18:04:32Z

docs/ble.md

 ```

-API Documentation
-----------------
+BLE-supported Events


The term "BLE-supported" doesn't make sense to me. See how they list events in the Node dgram API - they go in the same "Class: xyz" section as the APIs below.

@grgustaf -- my tool could easily(!) be adapted to check for WebIDL syntax. In my copious spare time, I'll gen up a simple app -- but I'll need someone who knows (useful) things to put it into the automatic testing...

...If that's what we want; I had sort of just planned on periodically running my actual tool over the code as a natural result of my testing/development and keeping the WebIDL up to date that way...

In re: events (i.e., "ble-supported" events)

The problem here is that each event isn't really part of the BLE object; in some ways, it's bifurcating the documentation: on one hand, most of the docs describe the interface the user sees/manipulates, etc. -- but with events (and maybe other things?), it's really addressing concerns of the guys implementing the underlying behavior. I don't know that the text describing events even belongs in this documentation...(?)

This might be a misunderstanding? Those events are indeed part of the BLE object and the ZJS user can (and needs to) register functions to listen for them with the .on() method. For example, see this BLE sample.

The on() method comes in because BLE is an EventEmitter; perhaps this needs to be made more clear, but it does basically behave like Node now and this was an attempt to document things in the same way they do. Maybe we missed some connecting glue though. :)

grgustaf · 2018-01-30T18:19:16Z

docs/buffer.md


 ### new Buffer(size)
 * `size` *integer* Length in bytes of the new buffer.

 The `size` argument specifies the length in bytes of the array that the Buffer
 represents. If a negative length is passed, a 0-length Buffer will be returned.
-If the size is too long and there is not enough available memory, an error will
+If there is not enough available memory to allocate the Buffer, an error will


Again, thanks for these English improvements - nice to have more eyes on it!

jimmy-huang · 2018-01-30T18:17:09Z

docs/aio.md

 interface AIO {
    AIOPin open(AIOInit init);
 };

 dictionary AIOInit {
-    unsigned long pin;
+    (unsigned long or string) pin;


We did have a existing PR that had typescript before #1572, but that was providing auto-completetions, and I haven't figure out how to import it into the ide without conflicting with other .ts.d files, so i had to close the PR, but we do have similar apis.

jimmy-huang · 2018-01-30T18:25:22Z

docs/ble.md

-    Characteristic Characteristic(CharacteristicInit init);
-    Descriptor Descriptor(DescriptorInit init);
+    void setServices(PrimaryService[] services);
+    PrimaryService newPrimaryService(PrimaryServiceInit init);


is newXXX the standard in webidl to declare a constructor?

...no, I don't think there is a standard WebIDL way to declare a constructor of one object as a method of some other object.

...but given that that's what this was doing, I made up a consistent naming scheme and hoped it would suffice (it gets the idea across; I didn't explain it and you intuited its function!). As with almost anything I write, I'm not married to it and would welcome(!) better ideas.

For Web IDL please always refer to the editors draft - the stable one is very old: https://heycam.github.io/webidl/

PrimaryService newPrimaryService(PrimaryServiceInit init); is pretty weird

In old fashioned JS (no class) the constructor is a function which has the same name as the object, ie.

function PrimaryService(PrimaryServiceInit init) { this.name = init.name; }

So in Web IDL you would just write it as

[Constructor(PrimaryServiceInit init), Exposed=Window] interface PrimaryService { };

t-harvey · 2018-02-02T02:58:49Z

Guys,
Thanks to all three of you -- the detail-oriented suggestions will serve as a list of things I should check before checking in any more changes (I'll answer each of them inline, but the theme is: you're right, thanks for catching that, and I've checked in a better copy with your improvements :-)...My takeaways in terms of the big-picture answers to this pull request are:

Editing the documentation is okay, and the style that Geoff started with Buffer seems to be acceptable. I'll probably be able to edit 3-6 files per week (there are on the order of 25 or so to do), and we can decide to have one big ol' pull request, or periodic small ones. As I say, I'm getting ready to provide detailed comments, above, and I've checked in a new version so you can see the deltas -- as I said at the beginning, my lawyers haven't given me the final okay (only b/c they're slow, not b/c it's anything but a done deal), so this pull request can't go in for a while (anywhere from 1 day to 2-3 weeks from now would mesh with earlier experiences)... Remember that my commitment includes minimal changes to the actual API descriptions (e.g., ble has a number of correctness issues) -- someone else will have to do those changes. :-)
WebIDL vs. Typescript -- I agree that Typescript is cool, but my experience over the last year is that there are a ton(!) of cool ways of doing things, and that the most important idea to get things done is to pick one and then go get things done. (You should have seen my dismal year-end review for 2016; the preceding sentence pretty much summed it up... :-) The advantages of staying the course with WebIDL are: 1) it's an industry standard for web developers/iot folks (which is kind of ZJS's focus, after all), 2) it's (mostly) done, 3) it now has a person (me, obviously :-) who has a vested interest in maintaining it over time, 4) using WebIDL doesn't preclude anyone else from also adding Typescript et al. to the docs if they want.

kenchris · 2018-02-02T11:44:23Z

Just to give some comments.

To generate bindings etc Web IDL is the way to go, that is what it is designed for and what browsers use.

TypeScript definition files are super useful, even when not using TypeScript. Like in VSCode I can just add a comment like // @ts-check at the top of a JavaScript file and the TypeScript compiled will be used to check and identify a lot of issues in the JavaScript code - I use it all the time :-)

For external libraries you can include these .ts.d files like I have the following in package.json for projects using Web Bluetooth:

   "devDependencies": {
    "@types/web-bluetooth": "0.0.4",
    ...
   }

I think that offering the same would be quite nice. I guess it is probably doable to generate these ts.d files from Web IDL as well.

I found a few related issues here: microsoft/TypeScript#3027 and microsoft/TypeScript-DOM-lib-generator#101 microsoft/TypeScript-DOM-lib-generator#227 https://github.com/SaschaNaz/webidl-serializer

kenchris · 2018-02-02T11:58:46Z

docs/ble.md

-    PrimaryService PrimaryService(PrimaryServiceInit init);
-    Characteristic Characteristic(CharacteristicInit init);
-    Descriptor Descriptor(DescriptorInit init);
+    void setServices(PrimaryService[] services);


in Web IDL we don't use [], you will normally use sequence<PrimaryService> or FrozenArray<PrimaryService> depending on the exact use-case.

kenchris · 2018-02-02T12:01:49Z

docs/ble.md


 ```javascript
 // require returns a BLE object
 // var ble = require('ble');

-[NoInterfaceObject]
+[ReturnFromRequire]
 interface BLE: EventEmitter {
    void disconnect(string address);
    void startAdvertising(string name, string[] uuids, string url);


We also don't have strings in Web IDL, we have DOMString, ByteString and USVString. Thus here you probably want

startAdvertising(USVString name, sequence<DOMString>, USVString url)

kenchris · 2018-02-02T12:03:24Z

docs/ble.md

+interface Characteristic {
+    attribute ReadCallback onReadRequest;
+    attribute WriteCallback onWriteRequest;
+    attribute SubscribeCallback onSubscribe;


Very un-web like... should be onsubscribe all lowercase plus these should be nullable.

attribute SubscribeCallback? onsubscribe

kenchris · 2018-02-02T12:05:38Z

docs/ble.md

 callback FulfillReadCallback = void (CharacteristicResult result, Buffer data);
 callback FulfillWriteCallback = void (CharacteristicResult result);
 callback FulfillSubscribeCallback = void (Buffer data);

+enum CharacteristicResult { "RESULT_SUCCESS", "RESULT_INVALID_OFFSET", "RESULT_INVALID_ATTRIBUTE_LENGTH", "RESULT_UNLIKELY_ERROR" } ;


When enumeration types are for strings (preferred today) then they should all be LOWER CASE and with HYPHEN between.. not underscore.

enum CharacteristicResult { "success", "invalid-offset"... }

kenchris · 2018-02-02T12:08:00Z

docs/buffer.md

+                             optional long length = this.length-offset,
+                             optional string encoding = "utf8");
+    long writeUInt8(octet value, unsigned long offset);
+    long writeUInt16BE(unsigned short value, unsigned long offset);


why not optional unsigned long offset = 0 here?

grgustaf · 2018-02-02T16:38:58Z

Part of the detail-oriented nature of my comments was that I didn't notice your comments about requesting just general discussion the first time I read it. Details are what we normally do around here. :)

But yes, absolutely editing the documentation is okay. We try to keep them up to date but that's not always in the forefront of our minds. And yes, if you want correct WebIDL then by all means we're willing to accept improvements. We'll try to understand the changes you're making so that we can make new stuff better out of the gate and maintain things properly. We haven't cared to date because basically it just served as a documentation format; specific correctness wasn't important and the couple times I looked into the spec didn't overwhelm me with a desire to read the whole thing. :)

grgustaf · 2018-04-27T16:54:46Z

Could still consider this, but closing for lack of progress... If we reopen I'd probably want to move webidl out into separate files like I proposed at some point, though.

Timothy Harvey added 3 commits January 24, 2018 16:11

jfjfjf

6b7e6c6

Reinserted WebIDL for buffer.md; changed the WebIDL in aio and ble to

b2e4ff7

be syntactically correct and to follow the new style of buffer.md. Added a new file that explains some of the differences between WebIDL and Javascript.

Merge branch 'working'

073a9a5

Some changes to a few .md files to get feedback.

t-harvey mentioned this pull request Jan 28, 2018

I'd like to fix the WebIDL in the documentation -- thoughts? #1759

Open

brianjjones added the work in progress label Jan 29, 2018

grgustaf suggested changes Jan 30, 2018

View reviewed changes

kenchris reviewed Jan 30, 2018

View reviewed changes

grgustaf reviewed Jan 30, 2018

View reviewed changes

jimmy-huang reviewed Jan 30, 2018

View reviewed changes

Timothy Harvey added 2 commits February 1, 2018 20:33

Made changes based on the first round of pull request 1800

3dce399

Merge branch 'working'

5ee32ce

kenchris reviewed Feb 2, 2018

View reviewed changes

grgustaf mentioned this pull request Apr 27, 2018

Web IDL in documentation is not compliant Web IDL #1822

Open

grgustaf closed this Apr 27, 2018

t-harvey mentioned this pull request Jul 2, 2018

Update WebIDL and modify documentation to fit new "node.js-like" style, part 2 #1892

Closed

t-harvey mentioned this pull request Jul 3, 2018

Update WebIDL and modify documentation to fit new "node.js-like" style, Part 2 #1893

Merged


		Closes the AIOPin. Once it is closed, all event handlers registered will no
		Closes the AIOPin. Once it is closed, all registered event handlers will no

Update WebIDL and modify documentation to fit new "node.js-like" style #1800

Update WebIDL and modify documentation to fit new "node.js-like" style #1800

Conversation

t-harvey commented Jan 28, 2018

grgustaf left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

grgustaf commented Jan 30, 2018

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

grgustaf left a comment • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

kenchris Feb 2, 2018 • edited Loading

Choose a reason for hiding this comment

t-harvey commented Feb 2, 2018

kenchris commented Feb 2, 2018

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

grgustaf commented Feb 2, 2018

grgustaf commented Apr 27, 2018

grgustaf left a comment •

edited

Loading

kenchris Feb 2, 2018 •

edited

Loading