Better JSDoc Types: Emit @typedefs, use standard primitive types, & more #13

jvilk · 2016-08-21T03:51:57Z

This PR changes the following in the Javascript generator, and addresses this issue in the Dropbox JS SDK repository:

Changes

Changes to js_client generator:

Changes primitives to use lowercase type names, as in the JSDoc documentation.
- Conveniently, TypeScript uses the same lowercase type names for primitives.
Changes Void type to void rather than null.
- While JSDoc doesn't have an official void type, ESLint's valid-jsdoc linting rule m the void type. It also lines up with TypeScript's void type.
Changes routes to return a Promise object, with success and error types (e.g. Promise.<FilesMetadata, Error.<FilesAlphaGetMetadataError>>)
List types are now emitted as Array.<ComposedType> rather than just Array.
Timestamps are emitted as Timestamp, which it JSDoc @typedefs to string
Moved -e parameter to the new js_types generator...

Adds a js_types generator that:

Generates JSDoc typedefs for referenced struct and union types.
Nullable fields on typedefs are now emitted as "optional" fields.
Takes in --extra-args to add extra parameters to route argument types.

Possible Improvements

Emit @typedef for alias types. For now, we unwrap all aliases.
Include stone attributes in field / type descriptions, e.g. regular expressions on string types.

Example Output

Here is the Dropbox Javascript SDK documentation and here is the SDK route/typedef code generated with these generator changes.

Since this is a rather large change to your generator, I'm expecting you to have plenty of comments and nits regarding the code I've written. Python isn't my primary language, so it's possible that some of my code isn't idiomatic / doesn't follow your style guide.

Let me know what you think!

jvilk · 2016-08-22T16:50:36Z

RE:Union types, It looks like the Java SDK, which currently doesn't have an open source generator, has deserialization logic that treats "collapsed" and "non-collapsed" unions differently.

The HTTP documentation shows all error types, which are all union types AFAIK, as having the following structure:

{
  "error_summary": "string",
  "error": {
    ".tag": "union tag",
    // extra properties on this particular tag, if any
  }
}

But error_summary isn't a field mentioned in the Stone data types, and .tag isn't documented elsewhere for union types. So I'm still not completely sure how to treat them when generating JSDoc.

braincore · 2016-08-23T06:58:17Z

Elated to see this, but it will take me some time to review this week.

To answer your last question, we should have a type definition like this:

struct Error<T>  # T is the error type for the route
    error_summary String
    error T
    user_message UserMessage

struct UserMessage
    text String
    locale String

However, we don't because stone doesn't have support for generics. To get around this, these types are simply handwritten into our SDKs and thus absent from our specs.

braincore · 2016-08-23T07:06:49Z

Re union types: Specifying a union variant with an object that has a .tag key will always work. The string version that you've seen is a "compact form" that only works for variants of void type.

We introduced the compact form to make it easier to specify when using a command-line tool like curl or httpie. We also did it for aesthetic reasons, which is why you'll see our documentation page use it for example arguments, but not for results; we always return the verbose form.

There's a short blurb here in stone's documentation.

braincore · 2016-08-23T07:12:04Z

Is it JSDoc or TypeScript that isn't happy with having multiple typedef files?

braincore · 2016-08-23T07:40:09Z

stone/target/js_client.py

@@ -4,6 +4,8 @@
 import json
 import six
 import sys
+import os


Alphabetize imports.

jvilk · 2016-08-23T16:10:41Z

Is it JSDoc or TypeScript that isn't happy with having multiple typedef files?

Multiple typedef files are fine, but we need a mechanism to avoid re-emitting the same type twice in different files. In the case of Dropbox, there are certain structs that are shared amongst the team and non-team routes. Simply emitting every typedef referenced by the routes would result in typedef duplicates.

Also, perhaps we should use the same pattern that's used for python and swift. That is, we have two generators: python_types and python_client. The former is invoked once to generate all types (in this case, we would just generate the typedefs). The latter is invoked twice (once for user routes, once for team routes).

That sounds like a great idea! I'll do that.

jvilk · 2016-08-23T20:37:42Z

@braincore / @rileytomasek I have updated the PR with a new js_types generator. The code is much cleaner now, and union types are properly emitted.

I updated the PR description, and linked to new documentation / JS files generated with these changes.

Let me know what you think!

jvilk · 2016-08-23T20:38:40Z

doc/builtin_generators.rst

@@ -29,21 +29,21 @@ command-line interface (CLI)::
    usage: stone [-h] [-v] [--clean-build] [-f FILTER_BY_ROUTE_ATTR]
                 [-w WHITELIST_NAMESPACE_ROUTES | -b BLACKLIST_NAMESPACE_ROUTES]
                 generator output [spec [spec ...]]
-


I configured Visual Studio Code to auto-remove extraneous whitespace at the end of lines when saving. Sorry for cluttering up the PR! lmk if I should reintroduce this whitespace.

jvilk · 2016-08-24T18:16:32Z

@braincore js_types is not emitting Dropbox's --extra-args data properly. While debugging, I notice that route.attrs is empty on all routes, preventing _parse_extra_args from working properly.

Do I need to toggle some type of generator configuration option to enable route.attrs?

jvilk · 2016-08-25T00:57:10Z

I fixed a few bugs, and updated the documentation / generated code links in the original post.

braincore · 2016-08-25T08:10:46Z

@jvilk: You'll need to use the -a argument to stone. Only then are the specified route attributes available to generators.

jvilk · 2016-08-25T16:57:09Z

@braincore Ahh, I overlooked that before. OK, I verified that the generator properly processes extra arguments and adds them to the relevant data type(s).

jvilk · 2016-09-01T16:00:25Z

poke I'm assuming everyone is busy. Let me know if there's anything else I should do for this PR (besides squash the commits once reviewed).

braincore · 2016-09-05T23:37:31Z

stone/target/js_types.py

+"""
+
+
+class JavascriptTypeGenerator(CodeGenerator):


nit: Type -> Types

braincore · 2016-09-06T02:03:56Z

Sorry for the delay. I just hooked this up into my local dropbox-sdk-js and re-generated the jsdocs, and it's a great improvement.

The one omission I see is struct polymorphism. For example, the output of filesGetMetadata is listed as FilesMetadata. But really, the output will be either FilesFileMetadata, FilesFolderMetadata, or FilesDeletedMetadata (all of which in the stone spec share Metadata as a base class). This isn't conveyed in the documentation and can be confusing.

braincore · 2016-09-06T02:04:39Z

Once you address struct polymorphism, and the minor n its, I think we'll be good to go! Thanks again!

jvilk · 2016-09-06T04:47:25Z

The one omission I see is struct polymorphism.

If I understand correctly: I'll have to check if any of a type's parent types is polymorphic. If it is, the type has an implicit .tag field. Then, when emitting routes, I'll have to check if the type is polymorphic; if it is, I emit an OR of all possible subtypes.

Let me give this a shot...

jvilk · 2016-09-06T05:24:14Z

Actually, this is a tad confusing. You're saying that FilesFileMetadata will look like the following?

{
  ".tag": "file",
  "name": ...,
  "id": ...,
   (etc)
}

In a standard stone union, .tag points to a field name that contains the union variant's data. In this case, .tag doesn't point to any field, but instead dictates which subtype it is.

Is this interpretation correct?

braincore · 2016-09-06T05:41:43Z

Yes, that's correct. The (short) serialization docs for them are here: https://github.com/dropbox/stone/blob/master/doc/json_serializer.rst#L71

How well will this map to JSDocs?

jvilk · 2016-09-06T15:17:11Z

@braincore Well, there's two ways to handle this.

Make the parent type have all of the child type's fields as optional fields. The docs for each field will mention which .tag makes it appear.
Make each child type have a .tag field. Since they are the child of one of these polymorphic structs, they always have a .tag field set to some value (or values if they are the child of multiple polymorphic structs, I guess). Make the routes that return closed unions return the union of all of the subtypes. Make the routes that return an open union return the union of all of the subtypes with the parent type.

I think the second option is better.

jvilk · 2016-09-06T22:58:34Z

@braincore OK, I've addressed the nits, and added support for structural polymorphism! The links to generated docs / JavaScript have been updated in the OP.

jvilk · 2016-09-06T23:09:07Z

@braincore btw, with this change, structs with enumerated subtypes that are a closed union (which I believe is the criteria for is_catch_all to be false?) are never directly referenced in the output JSDoc, since any reference to them (via field, argument, error type...) is replaced with a union of all of its subtypes. FilesMetadata is an example.

In this case, should I not emit a typing for the struct?

braincore · 2016-09-07T07:24:14Z

stone/target/js_types.py

+
+                jsdoc_tag_union = fmt_jsdoc_union(tag_values)
+                self.emit_wrapped_text('@property {%s} .tag - Tag identifying the subtype variant.' % jsdoc_tag_union)
+            else:


This case is unnecessary. If one of the subtypes is directly referenced, then there will be no .tag attribute since it isn't ambiguous. An example of this is the filesUpload route. It will return a FilesFileMetadata object (you can only upload a file, not a folder), and there's no .tag attribute.

Also, we don't support multiple layers of inheritance with enumerated subtypes. It's always only a single root struct and one layer of children (we might expand the design later).

It's always only a single root struct and one layer of children

OK; some of the helper functions in the code seemed to assume otherwise, so I went with a general design.

If one of the subtypes is directly referenced, then there will be no .tag attribute since it isn't ambiguous.

Oh, that complicates things. I could do the following:

Make .tag an optional field on these data types.

Make a second data type for FilesFileMetadataVariant or something that has .tag, while FilesFileMetadata does not.

Ignore the .tag option entirely.

I think 1 is the cleanest solution. I can put a comment, like "Tag identifying this subtype variant. This field is only present when needed to discriminate between multiple possible types."

Also, unless you have a good reason for me not to, I think I'll keep the more general code in there as it seems consistent with other design choices. For example, is_member_of_enumerated_subtypes_tree recursively climbs up the inheritance hierarchy.

Okay, that's fine :)

braincore · 2016-09-07T07:25:17Z

Verified that the enumerated subtypes output looks correct in JSDocs! Last bit is to remove some unnecessary logic, and then I think we're ready to merge this!

jvilk · 2016-09-07T15:14:40Z

OK, I think I came up with a useful compromise. I don't think the logic is unnecessary. When the docs points to FilesFileMetadata | FilesFolderMetadata as a route's return value, a developer needs to know how to discriminate between the two. Omitting the .tag field on those types entirely would be confusing in those cases.

As a compromise, I made it an optional field with a clarifying description. Does this work for you, or do you have a better idea in mind?

EDIT: I didn't update the docs at the top after this change, since it's so minor.

braincore · 2016-09-09T00:29:32Z

Haven't had a chance to test this today. But in preparation for landing, please squash your commits and write a descriptive commit log.

braincore · 2016-09-09T08:14:19Z

Just tested. Looks great! Just waiting for the squash.

@typedef

This commit adds a js_types generator that generates JSDoc @typedefs for Stone struct and union types. JSDoc annotations for routes, which the js_client generator generates, now reference these new @typedefs. As a consequence of this change, I have migrated the --extra-args parameter from js_client to js_types, since that parameter augments specific types with additional fields. Below, I detail how the generator maps each Stone data type to JSDoc. Namespaces ---------- To preserve namespaces, each user-defined type is prefixed with its Pascal-cased namespace. For example, the Metadata struct in the files namespace is emitted as FilesMetadata. Basic Types ----------- Most basic types are emitted as-before, except now using lowercase type names (e.g., number instead of Number). This convention is consistent with JSDoc's own documentation (see http://usejsdoc.org/tags-type.html), as well as alternative JavaScript type systems (e.g., TypeScript). A few other adjustments have been made, however: * Void type is emitted as void rather than null. * JSDoc does not have an official void type, but ESLint's valid-jsdoc linting rule recognizes the void type. This is also consistent with TypeScript. * valid-jsdoc documentation: http://eslint.org/docs/rules/valid-jsdoc * List types are emitted as Array.<ElementType> rather than just Array. * Timestamps are emitted as Timestamp, which is JSDoc @typedef'd to string. * Previously, these were incorrectly emitted as "Object". Aliases ------- All aliases are unwrapped and emitted as their base type. If desired in the future, every alias can be @typedef'd into their actual type, but that may be confusing to developers perusing the documentation. Structs ------- Each struct is mapped to a JSDoc @typedef. Each field is emitted as a @Property on that @typedef. Unfortunately, JSDoc typedefs do not support inheritance like JSDoc @classes, so every struct @typedef contains the fields from the struct's parent type(s). (Note: @Class would be inappropriate to use for Stone data types, as these are expected to be nominal types backed by a constructor.) More information on typedefs here: http://usejsdoc.org/tags-typedef.html Unions ------ Each union is mapped to a JSDoc @typedef with the appropriate fields. All unions have a .tag property that contains a string value indicating the union's variant. The JSDoc lists the type of .tag as the union of all known string values (e.g., 'foo' | 'bar'). Strictly speaking, open unions may have more possible values than listed; should this subtlety become an issue, the generator could be modified to emit the generic string type in the union (e.g., 'foo' | 'bar' | string). All of the fields on the union that are only present on a particular variant are marked as an optional @typedef @Property, with a description mentioning which .tag value it is tied to. If a union uses inheritance, the @typedef simply incorporates all of the options from the parent type(s). Struct Polymorphism ------------------- If a struct enumerates its subtypes, the generator emits an annotation for the struct's .tag field that lets developers discriminate between the possible subtypes. Like with unions, the .tag field's type is a union of all known possible values (e.g., 'foo' | 'bar'). All subtypes contain an *optional* .tag field typed with that subtype's tag value, since the .tag field may be omitted when the subtype is referenced directly. Any references to a struct that enumerates its subtypes are replaced with a union of all possible subtypes. For example, consider the following Stone types: ``` struct Resource union file File folder Folder path String struct ResourceHolder value Resource ``` In the JSDoc, the `value` field on `ResourceHolder` is typed as `File | Folder | Resource`. If `Resource` were a closed union, then it would be typed as `File | Folder`. Nullable Types -------------- Nullable types are mapped to optional parameters, indicating that the particular field may or may not be present on the data type. Routes ------ js_client emits a method for each route that accepts a single parameter corresponding to that route's Arg type and returns a Promise that resolves to the route's Result type (or rejects with the route's Error type, wrapped in a hardcoded Error object). js_client now emits proper JSDoc annotations for each of these types. The arg parameter references the appropriate type name emitted by js_types, and the return value is a Promise parameterized with the result type and the error type (`Promise.<ResultType, Error.<ErrorType>>`), where `Error` is the hardcoded object that wraps an API error.

jvilk · 2016-09-09T16:43:22Z

Squashed with descriptive commit message! (Which could probably be spun off into a doc if you feel like maintaining the description.)

braincore · 2016-09-09T20:53:14Z

Thanks for the detailed commit message! Rebased in. I'll be updating the Dropbox JS SDK soon.

braincore reviewed Aug 23, 2016
View reviewed changes

stone/target/js_client.py

@@ -4,6 +4,8 @@

import json

import six

import sys

import os

Copy link

Contributor

braincore Aug 23, 2016

Choose a reason for hiding this comment

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

Alphabetize imports.

jvilk reviewed Aug 23, 2016
View reviewed changes

braincore reviewed Sep 5, 2016
View reviewed changes

stone/target/js_types.py

"""

class JavascriptTypeGenerator(CodeGenerator):

Copy link

Contributor

braincore Sep 5, 2016

Choose a reason for hiding this comment

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

nit: Type -> Types

braincore reviewed Sep 7, 2016
View reviewed changes

jvilk force-pushed the better-jsdoc-types branch from 4e79a61 to 16f6afb Compare September 9, 2016 16:39

jvilk force-pushed the better-jsdoc-types branch from 16f6afb to e638edd Compare September 9, 2016 16:42

braincore closed this Sep 9, 2016

braincore reopened this Sep 9, 2016

braincore merged commit fd5ac46 into dropbox:master Sep 9, 2016

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Better JSDoc Types: Emit @typedefs, use standard primitive types, & more #13

Better JSDoc Types: Emit @typedefs, use standard primitive types, & more #13

jvilk commented Aug 21, 2016 •

edited

Loading

jvilk commented Aug 22, 2016 •

edited

Loading

braincore commented Aug 23, 2016

braincore commented Aug 23, 2016

braincore commented Aug 23, 2016

braincore Aug 23, 2016

jvilk commented Aug 23, 2016 •

edited

Loading

jvilk commented Aug 23, 2016

jvilk Aug 23, 2016 •

edited

Loading

jvilk commented Aug 24, 2016

jvilk commented Aug 25, 2016 •

edited

Loading

braincore commented Aug 25, 2016

jvilk commented Aug 25, 2016

jvilk commented Sep 1, 2016

braincore Sep 5, 2016

braincore commented Sep 6, 2016

braincore commented Sep 6, 2016

jvilk commented Sep 6, 2016 •

edited

Loading

jvilk commented Sep 6, 2016 •

edited

Loading

braincore commented Sep 6, 2016

jvilk commented Sep 6, 2016 •

edited

Loading

jvilk commented Sep 6, 2016

jvilk commented Sep 6, 2016 •

edited

Loading

braincore Sep 7, 2016

jvilk Sep 7, 2016

jvilk Sep 7, 2016

braincore Sep 8, 2016

braincore commented Sep 7, 2016

jvilk commented Sep 7, 2016 •

edited

Loading

braincore commented Sep 9, 2016

braincore commented Sep 9, 2016

jvilk commented Sep 9, 2016

braincore commented Sep 9, 2016

Better JSDoc Types: Emit @typedefs, use standard primitive types, & more #13

Better JSDoc Types: Emit @typedefs, use standard primitive types, & more #13

Conversation

jvilk commented Aug 21, 2016 • edited Loading

Changes

Possible Improvements

Example Output

jvilk commented Aug 22, 2016 • edited Loading

braincore commented Aug 23, 2016

braincore commented Aug 23, 2016

braincore commented Aug 23, 2016

braincore Aug 23, 2016

Choose a reason for hiding this comment

jvilk commented Aug 23, 2016 • edited Loading

jvilk commented Aug 23, 2016

jvilk Aug 23, 2016 • edited Loading

Choose a reason for hiding this comment

jvilk commented Aug 24, 2016

jvilk commented Aug 25, 2016 • edited Loading

braincore commented Aug 25, 2016

jvilk commented Aug 25, 2016

jvilk commented Sep 1, 2016

braincore Sep 5, 2016

Choose a reason for hiding this comment

braincore commented Sep 6, 2016

braincore commented Sep 6, 2016

jvilk commented Sep 6, 2016 • edited Loading

jvilk commented Sep 6, 2016 • edited Loading

braincore commented Sep 6, 2016

jvilk commented Sep 6, 2016 • edited Loading

jvilk commented Sep 6, 2016

jvilk commented Sep 6, 2016 • edited Loading

braincore Sep 7, 2016

Choose a reason for hiding this comment

jvilk Sep 7, 2016

Choose a reason for hiding this comment

jvilk Sep 7, 2016

Choose a reason for hiding this comment

braincore Sep 8, 2016

Choose a reason for hiding this comment

braincore commented Sep 7, 2016

jvilk commented Sep 7, 2016 • edited Loading

braincore commented Sep 9, 2016

braincore commented Sep 9, 2016

jvilk commented Sep 9, 2016

braincore commented Sep 9, 2016

jvilk commented Aug 21, 2016 •

edited

Loading

jvilk commented Aug 22, 2016 •

edited

Loading

jvilk commented Aug 23, 2016 •

edited

Loading

jvilk Aug 23, 2016 •

edited

Loading

jvilk commented Aug 25, 2016 •

edited

Loading

jvilk commented Sep 6, 2016 •

edited

Loading

jvilk commented Sep 6, 2016 •

edited

Loading

jvilk commented Sep 6, 2016 •

edited

Loading

jvilk commented Sep 6, 2016 •

edited

Loading

jvilk commented Sep 7, 2016 •

edited

Loading