Set up doc-comments and doxygen #34

jasongin · 2017-05-03T19:11:43Z

Add doc-comments to a few of the classes in napi.h: Value, String, Error
Add a doxygen config file

To generate documents:

Download and install doxygen from http://www.stack.nl/~dimitri/doxygen/download.html
Run the "doxywizard" (GUI) or doxygen.exe (command-line) using the Doxyfile configuration file added in this change.

Doxygen supports multiple different doc-comment styles and text formatting methods. Here, I have chosen:

Triple-slash style doc-comments, because they make it easier to add end-of-line comments for method parameters, and for simple property-style methods that are common in these APIs.
Markdown for text formatting, because we're all familiar with markdown and it's nicer to read compared to the @command or \command formatting instructions that are also supported. We can still mix in any of Doxygen's supported commands.

Doxygen also supports Javadoc style comments (/** ... */) but those would not be as nice here IMO.

jasongin · 2017-05-03T20:30:13Z

While someone has created a doxygen npm package, it apparently isn't widely used. I tried it out and it doesn't work very well: there's no way to download the doxygen binaries only when needed (so it's very slow when running repeatedly), and it doesn't show progress while downloading or running.

jasongin · 2017-05-03T21:01:23Z

Related: #6 Write documentation for C++ APIs

boingoing

Looks good to me, comments are mostly just nitpicky grammar and such.

Only glanced over the Doxyfile. This is a huge file.

👍

boingoing · 2017-05-03T21:07:16Z

napi.h

+    /// Converts to another type of `Napi::Value`, when the actual type is known or assumed.
+    ///
+    /// This conversion does NOT coerce the type. Calling any methods inappropriate for the actual
+    /// value type will cause `Napi::Error`s to be thrown.


Just nitpicky, should we write this as such?

Calling any methods inappropriate for the actual value type will throw Napi::Error.

boingoing · 2017-05-03T21:10:10Z

napi.h

+    /// Creates a new String value from a UTF-8 encoded C string.
+    static String New(
+      napi_env env,     ///< N-API environment
+      const char* value ///< UTF-8 encoded C string


C string seems to imply "null-terminated" but should we explicitly mention that here since there is an overload which takes a length parameter?

boingoing · 2017-05-03T21:11:02Z

napi.h

+    static String New(
+      napi_env env,      ///< N-API environment
+      const char* value, ///< UTF-8 encoded C string
+      size_t length      ///< length of the string in bytes


length is not capitalized, seems like it should be to be consistent with other comments.

boingoing · 2017-05-03T21:15:51Z

napi.h

+  /// may be changed to/from a strong reference by adjusting the refcount.
+  ///
+  /// The referenced value is not immediately destroyed when the reference count is zero; it is
+  /// merely then eligible for if there are no other references to the value.


Eligible for destruction?

boingoing · 2017-05-03T21:18:45Z

napi.h

+  ///         Napi::Value GetSomething(const Napi::CallbackInfo& info);
+  ///         void SetSomething(const Napi::CallbackInfo& info, const Napi::Value& value);
+  ///         Napi::Value DoSomething(const Napi::CallbackInfo& info);
+  ///       }


Nitpicky: This brace is not aligned with the class statement - looks like it's 2 spaces further right.

jasongin · 2017-05-03T21:34:05Z

Thanks @boingoing. I pushed an update that fixed all the issues you commented on.

jasongin · 2017-05-03T21:52:01Z

I was thinking an easy way to publish the generated API docs would be to use GitHub Pages. Based on the GitHub help, I created a gh-pages branch in this repo and pushed a commit with all the generated docs.

@mhdawson Can you go to the settings for this repo and enable GitHub Pages, using the gh-pages branch? Then I think the docs should be visible at https://nodejs.github.io/node-api/

jasongin · 2017-05-04T17:31:38Z

Docs are visible now at https://nodejs.github.io/node-api/

- Add doc-comments to a few of the classes in napi.h - Add a doxygen config file

jasongin · 2017-05-05T00:07:44Z

@digitalinfinity @gabrielschulhof @mhdawson @addaleax Any feedback on the documentation format?

addaleax · 2017-05-06T16:03:30Z

README.md

+### API Documentation
+
+ - [ABI-Stable C APIs in Node.js](https://nodejs.org/dist/latest-v7.x/docs/api/n-api.html)
+ - [C++ APIs in this package](https://nodejs.github.io/node-api/namespace_napi.html)


Aaaah this is so cool! Thank you for setting it up!

addaleax · 2017-05-06T16:04:21Z

README.md

@@ -5,6 +5,13 @@ Node.js API (N-API), along with library code that enables
 backward-compatibility with use with older versions of Node.js that do
 not have N-API built-in.

+### API Documentation
+
+ - [ABI-Stable C APIs in Node.js](https://nodejs.org/dist/latest-v7.x/docs/api/n-api.html)


latest-v7.x only works by accident (and it might/should get deleted) – I would just stick with linking to https://nodejs.org/api/n-api.html and accept that it only works once the 8.0.0 release is out

Yes. I wanted to link to https://github.com/nodejs/node/blob/master/doc/api/n-api.md for now, but links to external markdown files confuse the Doxygen generator.

addaleax · 2017-05-06T16:08:44Z

napi.h

+  /// wrapper automatically converts and throws it as a JavaScript exception.
+  ///
+  /// Catching a C++ exception of type `Napi::Error` also clears the JavaScript exception. Of
+  /// course it may be then re-thrown, which restores the JavaScript exception.


This paragraph sounds a bit confusing (it sounds like C++ exception management affects the JS VM’s state), I would be okay with just deleting it.

Reworded and combined with the previous paragraph.

addaleax · 2017-05-06T16:10:25Z

napi.h

+  /// a C++ exception of type `Napi::Error`.
+  ///
+  /// If a C++ exception of type `Napi::Error` escapes from a N-API C++ callback, then the N-API
+  /// wrapper automatically converts and throws it as a JavaScript exception.


By the way, should we catch all exceptions coming from C++ callbacks and convert them to JS errors? I don’t think there’s a way to do anything more meaningful (or at least I would assume JS engines don’t like it if the call stack unwinds outside of their control)

I'm not sure. I suppose we could catch any kind of std::exception and then copy the message into a JS error. But would that be safe, or could some exceptions leave the module in an inconsistent state?

The alternative is to just let any other kinds of uncaught C++ exceptions result in a crash, which is what happens now.

addaleax · 2017-05-06T16:10:59Z

napi.h

+  /// exception of type `Napi::Error`, until it is either caught while still in C++, or else
+  /// automatically re-thrown as a JavaScript exception when the callback returns to JavaScript.
+  ///
+  /// #### Example 2 - Ignoring a NAPI exception:


s/Ignoring/not catching/?

addaleax · 2017-05-06T16:11:12Z

napi.h

+  /// exception of type `Napi::Error`, until it is either caught while still in C++, or else
+  /// automatically re-thrown as a JavaScript exception when the callback returns to JavaScript.
+  ///
+  /// #### Example 3 - Handling a NAPI exception:


mhdawson · 2017-05-08T13:00:24Z

Sorry for the late response, I've been out all this week. Looks like you found somebody else to configure it. Regards, Michael (M.H.) Dawson Runtime Technologies Node.js Technical Lead Software Developer and Master Inventor Phone: 1-613-356-5484 | Phone: 1-343-882-2473 E-mail: [email protected] Find me on: 3755 Riverside Drive Ottawa, ON K1V 1B8 Canada From: Jason Ginchereau <[email protected]> To: nodejs/node-api <[email protected]> Cc: Michael Dawson/Ottawa/IBM@IBMCA, Mention <[email protected]> Date: 05/03/2017 05:52 PM Subject: Re: [nodejs/node-api] Set up doc-comments and doxygen (#34) I was thinking an easy way to publish the generated API docs would be to use GitHub Pages. Based on the GitHub help, I created a gh-pages branch in this repo and pushed a commit with all the generated docs. @mhdawson Can you go to the settings for this repo and enable GitHub Pages, using the gh-pages branch? Then I think the docs should be visible at https://nodejs.github.io/node-api/ — You are receiving this because you were mentioned. Reply to this email directly, view it on GitHub, or mute the thread.

jasongin force-pushed the doc branch from 38eaa8c to 19798fc Compare May 3, 2017 20:38

jasongin requested a review from boingoing May 3, 2017 20:39

jasongin force-pushed the doc branch from 19798fc to dbc568a Compare May 3, 2017 20:56

boingoing approved these changes May 3, 2017

View reviewed changes

jasongin added 2 commits May 4, 2017 17:04

Set up doc-comments and doxygen

4c1f5f5

- Add doc-comments to a few of the classes in napi.h - Add a doxygen config file

Updates for review feedback

f786523

jasongin force-pushed the doc branch from 9ad9959 to f786523 Compare May 5, 2017 00:05

addaleax approved these changes May 6, 2017

View reviewed changes

Address review feedback, fix bad merge

81f32fc

jasongin merged commit b62dc80 into nodejs:master May 9, 2017

jasongin deleted the doc branch May 9, 2017 19:02

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Set up doc-comments and doxygen #34

Set up doc-comments and doxygen #34

jasongin commented May 3, 2017

jasongin commented May 3, 2017

jasongin commented May 3, 2017

boingoing left a comment

boingoing May 3, 2017

boingoing May 3, 2017

boingoing May 3, 2017

boingoing May 3, 2017

boingoing May 3, 2017

jasongin commented May 3, 2017

jasongin commented May 3, 2017

jasongin commented May 4, 2017

jasongin commented May 5, 2017

addaleax May 6, 2017

addaleax May 6, 2017

jasongin May 8, 2017

addaleax May 6, 2017

jasongin May 8, 2017

addaleax May 6, 2017

jasongin May 8, 2017

addaleax May 6, 2017

jasongin May 8, 2017

addaleax May 6, 2017

jasongin May 8, 2017

mhdawson commented May 8, 2017 via email

Set up doc-comments and doxygen #34

Set up doc-comments and doxygen #34

Conversation

jasongin commented May 3, 2017

jasongin commented May 3, 2017

jasongin commented May 3, 2017

boingoing 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

jasongin commented May 3, 2017

jasongin commented May 3, 2017

jasongin commented May 4, 2017

jasongin commented May 5, 2017

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

mhdawson commented May 8, 2017 via email