549786 - Integration of API Documentation parser into the CFX Tools mini-server

Status: RESOLVED FIXED

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

Description

[Daniel Buchner [:dbuc]]

See Aza, Atul, and Brian Warner for further details

Comment 1

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

I think Brian Warner said he was going to take this on.

Comment 2

[Daniel Buchner [:dbuc]]

Yeah, he was using an email I was unaware of so I couldn't find it to assign it to him. Looks like :lastName is the golden ticket for that by what I see above :)

Comment 3

[Brian Warner [:warner :bwarner]]

As I understand it, here's what needs to be done (Aza in particular: please correct me!):

* the miniature webserver that is launched by "cfx docs" currently looks for .md files, parses them, and serves a processed form via HTTP to the JS doc-viewer frontend (which is also served by this server)
* this parsing code needs to be enhanced to use the "python-lib/cuddlefish/apiparser.py" functions, to parse JSdoc-like API docs (enclosed in <api> tags), and serve some processed form of the API docs to the frontend
* the JS frontend already(?) has code to handle this processed form and turn it into HTML for display

It sounded like it's more of an integration/moving-code-around task than parser-writing.

Comment 4

[Brian Warner [:warner :bwarner]]

More discussions with Aza:

* the backend server will parse any .md files into a list of sections. Each section is either a string of markdown, or a JS dictionary of parsed API docs. .md files that don't have any API sections will turn into list with a single string.
* we'll add a new /file -prefixed URL, which returns the raw backend file
* we'll change the /api -prefixed URL space to return a JSON-encoded list, of either MD strings or JSON objects (for API sections)
* we'll change the frontend JS to take these /api lists, perform markdown-to-HTML or JSAPI-to-HTML conversion on each as appropriate, merge the resulting strings, and display the merged results.
* other tools can fetch the /api JSON, discard the non-API sections, look up the methods, and do clever things like auto-complete method names and present context-sensitive docs.

Comment 5

[Brian Warner [:warner :bwarner]]

Thanks to a lot of help from Aza, I've pushed the integration work.. I'm not 100% sure about the new URL format, but everything else seems to work.

The new URLs are:

 /api/packages : returns a JSON structure with the package tree
 /api/packages/file/FILEPATH : returns a raw file, like foo.js
 /api/packages/docs/FILEPATH : returns a JSON-parsed API docs, from e.g. foo.md

It might be a good idea to change the way that works, so that raw files and parsed docs are served from URLs that don't start with "/api". In this case, the string "api" seems to be referring to "ways to control the server", as opposed to "docs for the API of javascript packages, which are being retrieved from this server". Not sure.

Comment 6

[Atul Varma [:atul]]

Apparently I was working on bug 551042 at the same time as you were working on this, and we made some of the same changes.

Since we can't actually release 0.2 without fixing bug 551042, I've gone ahead and backed out your changeset and pushed the bug 551042 fix.

Part of the aforementioned fix is to change the '/api/packages' path to be just '/packages', so your concerns in comment 5 should be taken care of.

Comment 7

[Brian Warner [:warner :bwarner]]

Attachment: WIP patch

here's the current work-in-progress patch. It updates the parser, adds some unit tests (for 'cfx testcfx'), and adds the client-side renderer. It also updates url.md to the new format.

Still to do: more unit tests, fix some known bugs in the parser (accept+require property names on returned objects, reject default values on required parameters, retain accurate whitespace on descriptions, and fix a drops-leading-prefix bug in descriptions).

Comment 8

[Brian Warner [:warner :bwarner]]

Attachment: WIP patch 2

here's the latest patch: I rewrote the parser completely and fixed all of the previously observed bugs. The biggest limitation right now is that parse errors in the .md file are not reported usefully to the 'cfx docs' frontend (you get an unhelpful error message instead of a stack trace). Use 'python python-lib/cuddlefish/apiparser.py PATH/TO/FOO.md' to manually parse a docs file until it is clean, then you can use 'cfx docs' to see it rendered.

Comment 10

[Brian Warner [:warner :bwarner]]

Attachment: final parser/renderer patch

I think this one is ready for review. It adds display of ParseErrors (with line numbers) so docs authors can find out where their docs have problems.

I'll attach a separate patch to fix the following:
 - 'cfx sdocs' should create+archive .md.json files
 - 'cfx sdocs' should not archive .md~ files (emacs backup files)

Comment 11

[Brian Warner [:warner :bwarner]]

Attachment: include .md.json files in the 'cfx sdocs' tarball

here's a second patch, on top of the first, which adds the pre-parsed .md.json files in the static tarball produced by 'cfx sdocs'. It also fixes a personal annoyance: copying "foo.md~" backup files (produced by emacs) into the tarball.

Comment 12

[Brian Warner [:warner :bwarner]]

Comment on attachment 436846 [details] [diff] [review]
include .md.json files in the 'cfx sdocs' tarball

Atul asked me to split the second patch out into a different bug, it now lives in Bug 557313

Comment 13

[Atul Varma [:atul]]

Attachment: Screenshot of new url module documentation

Comment 14

[Atul Varma [:atul]]

This looks great. I haven't finished reviewing yet, but here's two questions:

* Is the url.md supposed to be an actual replacement for the current docs for this module? There's currently a line of text that says "Thing in the middle", for instance... Wasn't sure if this file was meant to be included or not.

* Is there a way the JSON dict can optionally include information about where in the source MD docs some metadata was extracted from? This would make it possible in a post-processing step to, for instance, notice that a type specifier is invalid (e.g. @prop [foo] {stringu}) and raise an error pointing to where it's defined. If this is hard or there's bigger things we should be spending our time on, though, no worries.

* The actual styling of the url.md docs isn't quite ideal, but the underlying DOM structure is fine, so I expect that we can just twaak this after the code is committed.

Comment 15

[Atul Varma [:atul]]

For QA purposes, it'd be nice to eventually be able to ask for "all entities in documentation that have no description", such as the 'path' argument to url.fromFilename(). I expect this won't be very hard to implement, based on the code I've seen so far.

Comment 16

[Atul Varma [:atul]]

Comment on attachment 436826 [details] [diff] [review]
final parser/renderer patch

>+    var p = $("<span class='param'/>")
>+            .text(param.name) // what if param.name is empty?
>+            .appendTo($params);

I was playing around with this in the code and set a line of url.md to "@param {boolean}" and got this in the content area of the documentation:

  Oops, API docs renderer failed: TypeError: $("").text(param.name).appendTo is
  not a function

I looked at the code that displayed this:

>+          } catch (e) {
>+            $(where).html("Oops, API docs renderer failed: " + e);
>+          }

This is actually potentially dangerous, because the string representation of 'e' could contain HTML that could cause the page to mis-render or, worse, create security problems. Whenever you use $.html() you should make sure to sanitize/pre-escape anything passed in there; using $.text() instead in the above code would produce the same output in the general case, and not have any potential security vulnerabilities.

It may also be worth noting that in Firefox at least, some exception objects have filename and line number information:

  https://developer.mozilla.org/en/Core_JavaScript_1.5_Reference/Global_Objects/Error

In Jetpack-land we can just do "console.exception(e)" and it'll automatically figure out a stack trace; just being able to use Jetpack's "traceback" module would be nice here. For now though, I wonder if just re-throwing the error would be easiest, since it'd allow Firebug or the Error Console to catch the exception and log linenumber/filename information on its own.

Either way, not really a big deal, though it did leave me wondering where the error came from when I ran into it. :)

Still reviewing the rest of the code...

Comment 17

[Brian Warner [:warner :bwarner]]

(In reply to comment #14)

> * Is the url.md supposed to be an actual replacement for the current docs for
> this module? There's currently a line of text that says "Thing in the middle",
> for instance... Wasn't sure if this file was meant to be included or not.

Oops. Yes, it's meant to be a replacement (moving from the old manual styling
to the new APIdoc styling), but that "Thing in the middle" was a mistake.

> * Is there a way the JSON dict can optionally include information about where
> in the source MD docs some metadata was extracted from?

Yes, that wouldn't be too hard. The parser currently keeps track of the
initial line number for each section (to report accurate line numbers in
ParseError exceptions), and we could add that to the JSON dictionary. Maybe a
property named "line_number"?

> * The actual styling of the url.md docs isn't quite ideal, but the underlying
> DOM structure is fine, so I expect that we can just twaak this after the code
> is committed.

Yeah. I just copied some CSS from somewhere else (probably Aza), so I think
it wants a make-it-pretty pass.

Comment 18

[Atul Varma [:atul]]

Sweet. I think line_number is fine, unless we're naming everything else with interCaps or something. :)

It's also not something we need to do right now, unless it's ridiculously easy I guess!

Comment 19

[Atul Varma [:atul]]

Er, so one of the things we'd like to allow for is--unless there's a really compelling use case--the ability for any standards-compliant browser to view these docs, especially when they're hosted statically. Right now it looks like your code uses some SpiderMonkey-specific features that aren't supported in other browsers' JS engines. For example, when I load the url module documentation in Safari, I get:

  Oops, API docs renderer failed: ReferenceError: Left side of for-in
  statement is not a reference.

Also, I noticed you use the Iterator constructor in your code, which sadly isn't supported in Safari.

It shouldn't take much work to modify the code so it works fine on these other browsers; we don't need to test it out on all of them, I don't think--just making sure it works both on Firefox and one other standards-compliant browser is usually enough to make sure you're not inadvertently writing anything that will only work on Firefox.

Comment 20

[Atul Varma [:atul]]

By the way, as an alternative to Iterator, I think you can rely on Array.forEach() to exist in all modern standards-compliant browsers, since it's part of ES5:

  https://developer.mozilla.org/En/Core_JavaScript_1.5_Reference/Objects/Array/ForEach

So you don't have to resort to c-style for-loop nastiness. :)

Comment 21

[Atul Varma [:atul]]

> +    if( param.default ){
> +      $("<span class='default'/>")
> +        .text("=" + param.default)
> +        .appendTo(p);
> +    }

It might be useful here to just set the text of the element to param.default, and then use the "content" CSS property on the "default" class to prepend the "=":

  https://developer.mozilla.org/en/CSS/content

Not sure, though, if this is the kind of thing that one might one day want to change via CSS or not. Just a thought, and not a big deal in any case.

Similarly, this line might be more appropriate to do with CSS:

+    }
+
+    if( i != doc.params.length-1 ) p.after(", ");
+  });

But I'm not sure.

+  if( doc.returns ){
+    var $ret = $("<span class='returns'/>").text(" returns ");
+    $("<span class='type'/>").text(doc.returns.type).appendTo($ret);
+    $ret.appendTo($title);
+  }

Hm, so for purposes of eventual localization, it'd be nice to not have " returns " as a bare string here--this is one of the reasons all localizable code is currently in index.html. Not that I know exactly *how* we'd vary index.html per-locale, but I figured it'd be easier if such content were in the DOM with a particular ID. Feel free to totally disagree though!

+function _createDescription(doc){
+  // TODO: this is actually markdown
+  var $desc = $("<div class='description'/>")
+    .text( doc.description );
+  return $desc;
+}

Can we either make a bug for this TODO or resolve it in the next iteration of your patch?

Comment 22

[Atul Varma [:atul]]

Comment on attachment 436826 [details] [diff] [review]
final parser/renderer patch

Aside from the issues I've already mentioned, this looks great!

Comment 23

[Brian Warner [:warner :bwarner]]

Attachment: new version of parser/renderer patch

Issues addressed:
 * removed mistake from url.md
 * added "line_number" property to JSON form, for each API object
 * renders properly on Safari
 * "=" in default values comes from CSS, not hardcoded string
 * descriptions are fully markdown-parsed, including bugzilla links
 * $(where).html()-in-exception fixed, although if we want to safely allow rendering of malicious .md then we need to make a proper security review

Issues not addressed:
 * CSS improvements
 * ", " between arguments is still hardcoded
 * " returns " is still hardcoded (not L10N friendly)
 * handling of "@param {boolean}" (missing a name) still gives weird jQuery error

Issues raised but which don't need a fix:
 * comment15 (ask for all methods without a description): this is possible with the current JSON syntax, and easiest to do with a python-side "cfx checkdocs" or the like

I don't know what CSS offers to help with the inter-argument comma. I think I see how to use CSS to put "returns" in the right place, but I don't know if that would be any more L10N-friendly than the hardcoded string. I'm happy to defer CSS improvements for a later time.

Comment 24

[Brian Warner [:warner :bwarner]]

Oh, I should mention that to get the full markdown-rendering in descriptions, I had to move a couple of functions out into a separate "render.js" file. The old code was doing markdown rendering, but not applying the regexps to make "bug NNN" into a hyperlink. I'm not sure if doing those regexps is worth the hassle (or performance hit?) of having a separate file for them, or if there's an easier/better way to make that markdownToHtml() function available to renderapi.js (like attaching it to window or something).

Comment 25

[Atul Varma [:atul]]

Comment on attachment 437493 [details] [diff] [review]
new version of parser/renderer patch

Looks and works great, thanks! Can you push it?

Comment 26

[Brian Warner [:warner :bwarner]]

pushed. thanks!

Comment 27

[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".