605627 - Markdown syntax for API reference material needs to understand about classes

Status: RESOLVED FIXED

(Add-on SDK Graveyard :: General, defect)

Description

[Will Bamberg [:wbamberg]]

To provide properly structured API reference documentation the Markdown syntax needs to support the concept of a class. 

This bug has three parts:
    - extend the current API reference syntax so you can represent classes with it, and describe methods and properties as belonging to a class
    - update the toolchain (both renderapi.js and the AOB code)so it understands the new syntax and can emit the appropriate HTML
    - update the Markdown to include the new syntax

Comment 1

[Will Bamberg [:wbamberg]]

Attachment: Patch to add classes to API reference syntax

Comment 2

[Will Bamberg [:wbamberg]]

I've submitted a first attempt at this into my GitHub fork, at: http://github.com/wbamberg/jetpack-sdk/tree/bug605627, and attached the patch. 

 Summary:
--------
1) apiparser.py is reworked so as to understand classes. 
2) renderapi.js and the associated main.js are reworked so as to emit the new syntax. 
3) SDK's apidocs.css reworked a bit to improve readability

Details:
--------
1) apiparser.py changed to understand classes

This is done by adding a new "class" type and enabling you to nest complete "constructor", "method" and "property" types inside the ""class" type. So given a collection of api elements like this:

<api name="Thing">
@constructor
Creates a thing.
@param size {object}
  Size of the thing.
  @prop [width] {number}
    Width of the thing.
  @prop [height] {number}
    Height of the thing.
</api>

<api name="height">
@property {number}
Height of the thing.
</api>

<api name="width">
@property {number}
Width of the thing.
</api>

<api name="sendMessage">
@method
Send a message to another thing.
@param message {string,number,object,array,boolean}
The message to send.  
@param [anotherThing] {object}
The other thing.
</api>

...you can put them in a class just by wrapping them all inside a "class" type:

+ <api name="thing">
+ @class
+ Thing class. Does stuff.

<api name="Thing">
@constructor
Creates a thing.
@param size {object}
  Size of the thing.
  @prop [width] {number}
    Width of the thing.
  @prop [height] {number}
    Height of the thing.
</api>

<api name="height">
@property {number}
Height of the thing.
</api>

<api name="width">
@property {number}
Width of the thing.
</api>

<api name="sendMessage">
@method
Send a message to another thing.
@param message {string,number,object,array,boolean}
The message to send.  
@param [anotherThing] {object}
The other thing.
</api>

+ </api>

The JSON output for a class includes three keys: "constructors", "methods" and "properties" each of which is a list of those elements. 

It's _almost_ exactly backwards compatible with the previous parser: running both versions over the current set of MD files produces two differences:

a) in one file (list.md) the new parser locates an extra "\n". I don't think this is worth worrying about.

b) given MD input like this:

<api name="allow">
@property {object}
Permissions for the content, with the following keys:
@prop script {boolean}
  Whether or not to execute script in the content.  Defaults to true.
</api>

the existing parser will discard the embedded @prop and produce JSON like this:

["api-json", 
    {
    "line_number": 177, 
    "property_type": "object", 
    "type": "property", 
    "name": "allow", 
    "description": "Permissions for the content, with the following keys:"
    }
],

...while the new parser will embed the @prop:

["api-json", 
    {
    "line_number": 177, 
    "name": "allow", 
    "props": 
        [
            {
            "line_number": 180, 
            "required": true, 
            "type": "boolean", 
            "name": "script", 
            "description": "Whether or not to execute script in the content.  Defaults to true."
            }
        ], 
        "property_type": "object", 
        "type": "property", 
        "description": "Permissions for the content, with the following keys:"
    }
],

This looks to me like an inadvertent bug fix so I left it. This pattern occurs in 3 files: panel, loader, symbiont.

2) renderapi.js and the associated main.js are reworked so as to emit the new syntax. 

In this revision there are separate code paths for the old and new syntax. I expect this to be temporary and that in a couple of weeks when we've moved over the MD files then I can go back and take the old code path out. For that reason I haven't take the time to make this code very elegant, but have just got it working as quick as possible. 

Basically main.js looks at the JSON and if it contains a "class" type it assumes it's looking at the new syntax. 

The way it renders the new syntax is quite different and I think it's important for us to agree that it's appropriate. The current code, given a jumble of [markdown] and [api-json] elements, will render them in the order they are encountered. This means that the high-level structure of the doc is defined by the MD file itself. The new code separates out elements into three lists: [markdown] elements, [class] elements, and [function] elements, then renders each in turn. That means the docs all have a structure like:

Overview
--------
General descriptive text, intro, examples and stuff.

Classes
-------
Class 1
...
Class n

Global Functions
----------------
Function 1
...
Function n

This is a deliberate choice to try to make sure all the docs get a consistent structure.  But let me know if you don't like it: I guess it's a tradeoff between consistency and flexibility. 

3) SDK's apidocs.css reworked a bit to improve readability

These are really minimal changes just to make it readable and have a look and feel consistent with the old syntax. I do think there are further improvements that can be made here but believe the general issue of presentation shouldn't be in scope for this bug fix.

Comment 3

[Drew Willcoxon :adw]

(In reply to comment #2)
> This is a deliberate choice to try to make sure all the docs get a consistent
> structure.  But let me know if you don't like it: I guess it's a tradeoff
> between consistency and flexibility. 

Ship it!

Comment 4

[Myk Melez [:myk] [@mykmelez]]

Comment on attachment 485442 [details] [diff] [review]
Patch to add classes to API reference syntax

Drew: is that a feedback+?

Comment 5

[Drew Willcoxon :adw]

Oh, I was just reacting to the part I quoted, which is great.  Everything Will says in comment 2 sounds good to me, but I haven't looked at the patch.  Do you want me to?

Comment 6

[Drew Willcoxon :adw]

Comment on attachment 485442 [details] [diff] [review]
Patch to add classes to API reference syntax

I skimmed through the patch, and comment 2 sounds good to me, so feedback+ on the approach.  Thanks also for fixing the embedded @prop bug.  Brian would probably be best to review.  One minor thing: please always use spaces instead of tabs, since different people may have their tab stops set to different values, causing files to look different depending on who's viewing them.  (The indentation of main.js is off in Bugzilla's diff view for instance.)

Comment 7

[Will Bamberg [:wbamberg]]

Attachment: Updated patch: added [signature] key and fixed formatting

Comment 8

[Myk Melez [:myk] [@mykmelez]]

Comment on attachment 486725 [details] [diff] [review]
Updated patch: added [signature] key and fixed formatting

Switching review request to me, as Brian is at a conference most of the rest of the week.

Comment 9

[Will Bamberg [:wbamberg]]

Comment on attachment 486725 [details] [diff] [review]
Updated patch: added [signature] key and fixed formatting

You might want to wait for another update before reviewing this. I realize I didn't update the apiparser tests, sorry.

Comment 10

[Myk Melez [:myk] [@mykmelez]]

Comment on attachment 486725 [details] [diff] [review]
Updated patch: added [signature] key and fixed formatting

(In reply to comment #9)
> You might want to wait for another update before reviewing this. I realize I
> didn't update the apiparser tests, sorry.

Ok, no worries, cancelling the review request in the meantime.

Comment 11

[Will Bamberg [:wbamberg]]

Attachment: Updated patch: added updated to test_apiparser and TestRenderer

Updated apiparser tests and TestRenderer.

Comment 12

[Myk Melez [:myk] [@mykmelez]]

Comment on attachment 487967 [details] [diff] [review]
Updated patch: added updated to test_apiparser and TestRenderer

This looks good to me as well.  And I think the overall approach is fine, modulo some concern over the introduction of the concept of classes, which JavaScript doesn't really have.  But we need some term to identify the objects you can construct that have properties and methods, and "class" seems reasonable enough. r=myk

Comment 13

[Will Bamberg [:wbamberg]]

(In reply to comment #12)
> Comment on attachment 487967 [details] [diff] [review]
> Updated patch: added updated to test_apiparser and TestRenderer
> 
> This looks good to me as well.  And I think the overall approach is fine,
> modulo some concern over the introduction of the concept of classes, which
> JavaScript doesn't really have.  But we need some term to identify the objects
> you can construct that have properties and methods, and "class" seems
> reasonable enough. r=myk

Thanks. This is really my ignorance of JavaScript showing. Would it help to call them "Objects" not "Classes" in all the generated docs"?

Comment 14

[Myk Melez [:myk] [@mykmelez]]

(In reply to comment #13)
> (In reply to comment #12)
> > Comment on attachment 487967 [details] [diff] [review] [details]
> > Updated patch: added updated to test_apiparser and TestRenderer
> > 
> > This looks good to me as well.  And I think the overall approach is fine,
> > modulo some concern over the introduction of the concept of classes, which
> > JavaScript doesn't really have.  But we need some term to identify the objects
> > you can construct that have properties and methods, and "class" seems
> > reasonable enough. r=myk
> 
> Thanks. This is really my ignorance of JavaScript showing. Would it help to
> call them "Objects" not "Classes" in all the generated docs"?

Hmm, it might.  By way of comparison, the JavaScript Reference <https://developer.mozilla.org/en/JavaScript/Reference>, calls the set of things that are available by default in all JavaScript scopes "Global Objects" and then divides them into four categories: Constructors, Errors, Non-constructor Functions, and Other.

The reference pages for constructor objects like Date <https://developer.mozilla.org/en/JavaScript/Reference/Global_Objects/Date> then call them variously "objects" or "constructors" (which are both correct, since constructors are also objects) and describe both their own properties and methods (which are akin to class variables and static methods in class-based languages) as well as the properties and methods of their instances.

(In the SDK APIs, I don't think we define any properties and methods on constructors themselves at the moment, although we might do so in the future.)

However, my problem with calling these "objects" is that the word is so generic. After all, the global functions are also objects (since functions are first-class objects in JavaScript). And so are singletons like the simple-storage module's "storage".

In fact, the only exported symbols whose values are arguably not objects are primitive values like simple-storage's quotaUsage (which is a number); but even those get converted to objects (like Number) transparently whenever you access properties or call methods on them.

In JavaScript, almost everything is an object, so the "object" label just isn't that descriptive.

The most JavaScript-y way to describe these would probably be to call them constructors and name them after the constructor function itself (which has the corollary benefit that the thing being described is named exactly the same as the exported symbol).

It doesn't seem like that would require changes to the Markdown syntax; I'm not sure about the JSON intermediary format; but it would definitely require a change to the rendered output, since otherwise the output would represent a constructor as having a constructor, which would be strange.

Perhaps something like this would work:

  Constructors
  ------------
  
    Panel
    -----

      Parameters
      ----------
        ...
  
      Instance Properties
      -------------------
        ...
  
      Instance Methods
      ----------------
        ...
  
  Global Functions
  ----------------
    ...

Then, if we add properties and methods to a constructor itself at some point, we could list those as "Properties" and "Methods" (i.e. without "Instance") of the constructor to distinguish them from instance properties/methods.

And for singleton objects and primitive values like simple-storage's "storage" and "quotaUsage" symbols, perhaps we could simply not have a second-level heading and just list them first under the API Reference heading.

Thoughts?

Comment 15

[Drew Willcoxon :adw]

Although it's not technically correct, the term "class" is often used and understood around JS.  It immediately calls to mind a constructor that creates new instances and the properties and methods of those instances.  It's a shorthand.  "Constructor" is definitely not the right term since it refers narrowly to the function used to create new instances; we shouldn't overload it to stand in for the whole template.  If we want to be technically correct I'd rather use "prototype" or "type", but I think "class" is best.

Comment 16

[Myk Melez [:myk] [@mykmelez]]

Fair enough, I'm ok with that; we can always adjust as needed.

Comment 17

[Will Bamberg [:wbamberg]]

Attachment: Update to apiparser, test code, SDK rendering code and Markdown files

This patch includes updates the the Markdown files to support the extended syntax, plus a few more tweaks to the SDK rendering code. 

Git complains when I try to apply it:

patch605627-4:828: new blank line at EOF.
+
patch605627-4:1134: new blank line at EOF.
+
patch605627-4:2296: new blank line at EOF.
+
patch605627-4:2373: new blank line at EOF.
+
patch605627-4:3042: new blank line at EOF.
+
warning: squelched 1 whitespace error
warning: 6 lines add whitespace errors.

...but it's not obvious to me what the relation is between the line referenced, and the complaint, so I thought it would be best to submit it anyway for review and try to fix that later.

Comment 18

[Will Bamberg [:wbamberg]]

Attachment: Patch to update the AOB so it understands the new API syntax

Updates to the 'api' Django app that renders API reference docs, so it understands the new class syntax. This change also adds most, but not quite all, of the CSS readability improvements desired by bug605629. 

A couple of things:
- this code won't understand the old class syntax, so the patch should not be applied until after the other patch attached to this defect, which updates the Markdown files and apiparser. 

- I don't actually understand how this code can generate JSON from the Markdown files. The "from cuddlefish import apiparser" line at the top of "views.py" is commented out, but I don't understand how else this code is going to be able to use apiparser to generate the JSON. I had to uncomment this line to get the thing to work (although I didn't include that in this patch).

- git complains about a whitespace error when I try to apply the patch: I hope this patch can nevertheless be reviewed, and I can fix that tomorrow.

Comment 19

[Will Bamberg [:wbamberg]]

The AOB change is also on GitHub at: https://github.com/wbamberg/FlightDeck/tree/bug605627

Comment 20

[Myk Melez [:myk] [@mykmelez]]

(In reply to comment #17)
> Created attachment 488777 [details] [diff] [review]
> Update to apiparser, test code, SDK rendering code and Markdown files
> 
> This patch includes updates the the Markdown files to support the extended
> syntax, plus a few more tweaks to the SDK rendering code. 

I'd rather review the updates to the Markdown files in a separate patch with its own bug.  Can you submit an updated patch with just the changes in attachment 487967 [details] [diff] [review] along with any tweaks to it you've made since then?  Alternately, and perhaps preferably, I can land that patch, and then we can iterate with tweaks in a separate bug.

Comment 21

[Will Bamberg [:wbamberg]]

Attachment: Update apiparser and SDK rendering code only

This patch is just the changes to apiparser and the rendering code. apiparser is not changed from the last patch you reviewed, but I had to change the rendering code slightly so it would understand how to render module level properties. 

I did also take out the old code path to render the old syntax, since I was submitting the updated markdown anyway, and since it turns out that in a lot of cases it's not really possible to determine whether you're looking at the old syntax or the new syntax.

Comment 22

[Myk Melez [:myk] [@mykmelez]]

Comment on attachment 489173 [details] [diff] [review]
Update apiparser and SDK rendering code only

This still looks good, r=myk.

I took a closer look at code formatting this time and have some suggestions for improved readability and better conformance with Mozilla convention, although I recognize that some of these formatting issues are actually issues in the original code, and we can check in the patch without these changes.  See below for the details.


>+.api-heading{font-size: 200%; font-weight: bold;padding-top:30px;padding-bottom:10px;}
>+.api{margin-bottom:30px;}
>+.api>.name{font-size: 125%; font-weight: bold; padding:3px;border: solid 1px #EEE;background: #EEE;margin-bottom:10px;}

Nit: the CSS in these files would be easier to read with more whitespace and wrapping of long lines, i.e.:

  .api-heading {
    font-size: 200%;
    font-weight: bold;
    padding-top: 30px;
    padding-bottom: 10px;
  }
  .api { margin-bottom: 30px; }
  .api > .name {
    font-size: 125%;
    font-weight: bold;
    padding: 3px;
    border: solid 1px #EEE;
    background: #EEE;
    margin-bottom: 10px;
  }


>+    if (hunks.length == 0) return;

Nit: put line breaks after conditional statements, i.e.:

    if (hunks.length == 0)
      return;


>+    var markdown = new Array();
>+    var classes = new Array();
>+    var functions = new Array();
>+    var properties = new Array();

Nit: arrays are typically instantiated in Mozilla code using array literals ([]), i.e.:

    var markdown = [];
    var classes = [];
    var functions = [];
    var properties = [];


>+        apiHunksExist = true;
>+        if (hunk[1].type == "class") {
>+          classes.push(hunk);
>+        }
>+        else if (hunk[1].type == "function") {
>+          functions.push(hunk);
>+        }
>+        else if (hunk[1].type == "property") {
>+          properties.push(hunk);
>+        }

Nit: don't use parentheses around one line blocks if all blocks in the conditional contain only single statements, i.e.:

        if (hunk[1].type == "class")
          classes.push(hunk);
        else if (hunk[1].type == "function")
          functions.push(hunk);
        else if (hunk[1].type == "property")
          properties.push(hunk);


>+  if (doc.type == "property") {
>+    return _createPropertyTitle(doc, "Property: ");
>+  }
>+  else if (doc.type == "class") {
>+    return _createClassTitle(doc, "Class: ");
>+  }
>+  else {
>+    return _createMethodTitle(doc, false, "Function: ");
>+  }
>+}

Nit: use "if" instead of "else if" and leave out "else" after conditional statements that return, i.e.:

  if (doc.type == "property")
    return _createPropertyTitle(doc, "Property: ");

  if (doc.type == "class") {
    return _createClassTitle(doc, "Class: ");

  return _createMethodTitle(doc, false, "Function: ");

Alternately, in this case you could use a "switch" statement (leaving out the "break", since the statements return):

  switch(doc.type) {
    case "property":
      return _createPropertyTitle(doc, "Property: ");
    case "class";
      return _createClassTitle(doc, "Class: ");
    default:
      return _createMethodTitle(doc, false, "Function: ");
  }


>-  if( doc.returns ){
>+  if( doc.returns && showReturns){

Nit: put a space after "if".
Nit: put a space before the opening curly brace.
Nit: use spaces inside parenthesized expressions consistently, either a space after the opening parenthesis and before the closing one, or no space in either place (the latter of which is more consistent with Mozilla convention).


>+  for(var i = 0; i < components.length; i++) {

Nit: put a space after "for".

Comment 23

[Myk Melez [:myk] [@mykmelez]]

Comment on attachment 488777 [details] [diff] [review]
Update to apiparser, test code, SDK rendering code and Markdown files

Cancelling this review, which is no longer necessary.

Comment 24

[Myk Melez [:myk] [@mykmelez]]

Comment on attachment 488790 [details] [diff] [review]
Patch to update the AOB so it understands the new API syntax

Note: I have submitted bug 611173 to track this work, which seems better as a separate bug.

Comment 25

[Myk Melez [:myk] [@mykmelez]]

The Add-on SDK is no longer a Mozilla Labs experiment and has become a big enough project to warrant its own Bugzilla product, so the "Add-on SDK" product has been created for it, and I am moving its bugs to that product.

To filter bugmail related to this change, filter on the word "looptid".

Comment 26

[Myk Melez [:myk] [@mykmelez]]

Landed, without nits fixed, as https://github.com/mozilla/addon-sdk/commit/026b4e8e78336c2dbbf30edb14e5db78ca4afb21.