dx-spec icon indicating copy to clipboard operation
dx-spec copied to clipboard

Params

Open jamiebuilds opened this issue 7 years ago • 4 comments

The more I look at it the more I don't like the syntax for documenting params:

// - param: `a` `number` - description
// - param: `b` `string` - description
function foo(a, b) {
  // ...
}

Especially once you are using Flow/TypeScript:

// - param: `a` - description
// - param: `b` - description
function foo(a: number, b: string) {
  // ...
}

I think I would prefer doing this:

function foo(
  a, // `number` - description
  b, // `string` - description
) {
  // ...
}
function foo(
  a: number, // description
  b: string, // description
) {
  // ...
}

jamiebuilds avatar Nov 25 '17 00:11 jamiebuilds

Other examples:

function foo(
  opts: { // Options
    a: number, // description
    b: string, // description
  },
) {
  // ...
}
function foo(
  opts /*: { // Options
    a: number, // description
    b: string, // description
  } */,
) {
  // ...
}

jamiebuilds avatar Nov 25 '17 00:11 jamiebuilds

I'm into it - some of the intent for the JSDoc style of params-as-comments-above-the-thing is that you could document parameters that weren't really real, like

// @param {numbers...} a buncha numbers
function add() {
  var sum = 0;
  for (var i = 0; i < arguments.length; i++) sum += arguments[i];
  return sum;
}

From what I've seen, people did take advantage of this flexibility to have documentation document the intended API, not what the code really says literally. But we're taking a very different approach, and one that seems more sustainable. Inline docs make sense.

tmcw avatar Nov 27 '17 01:11 tmcw

We should probably have some amount of consistency about how comments are attached to nodes.

function foo(
  opts /*: { // Options
    a: number, // description
    b: string, // description
  } */,
) {
  // ...
}

With this syntax the // description in a: number, // description is actually attached to the object property b.

Basing it off of location rather than node attachment makes enough sense to me.

jamiebuilds avatar Nov 27 '17 02:11 jamiebuilds

@thejameskyle can you say more about what you don't like about being able to document params in the outer function comment?

Asking because I definitely dig the more 'inline' styles you're proposing here, with description comments colocated with the parameter, for many use cases. But something in me balks at the notion that the only way to document individual params would be to break up a function signature into multiple lines -- especially when one of the params requires a long explanation.

Here's an example from Mapbox GL JS:

    /**
     * Returns an array of [GeoJSON](http://geojson.org/)
     * [Feature objects](http://geojson.org/geojson-spec.html#feature-objects)
     * representing visible features that satisfy the query parameters.
     *
     * @param {PointLike|Array<PointLike>} [geometry] - The geometry of the query region:
     * either a single point or southwest and northeast points describing a bounding box.
     * Omitting this parameter (i.e. calling {@link Map#queryRenderedFeatures} with zero arguments,
     * or with only a `options` argument) is equivalent to passing a bounding box encompassing the entire
     * map viewport.
     * @param {Object} [options]
     * @param {Array<string>} [options.layers] An array of style layer IDs for the query to inspect.
     *   Only features within these layers will be returned. If this parameter is undefined, all layers will be checked.
     * @param {Array} [options.filter] A [filter](https://www.mapbox.com/mapbox-gl-style-spec/#types-filter)
     *   to limit query results.
     *
     * @returns {Array<Object>} An array of [GeoJSON](http://geojson.org/)
     * [feature objects](http://geojson.org/geojson-spec.html#feature-objects).
     * ... (more explanation, examples, etc.)
     */
    queryRenderedFeatures(geometry?: PointLike | [PointLike, PointLike], options?: Object) {

If param docs had to be inline, this would be something like:

     /* Returns an array of [GeoJSON](http://geojson.org/)[Feature objects](http://geojson.org/geojson-spec.html#feature-objects) representing
visible features that satisfy the query parameters. */
   queryRenderedFeatures(
      geometry?: PointLike | [PointLike, PointLike], /* The geometry of the query region: either
                a single point or southwest and northeast points describing a bounding box.
                Omitting this parameter (i.e. calling {@link Map#queryRenderedFeatures} with
                zero arguments, or with only a `options` argument) is equivalent to passing a
                bounding box encompassing the entire map viewport. */
     options: {
         filter?: Array, /* A [filter](https://www.mapbox.com/mapbox-gl-style-spec/#types-filter) to
                limit query results. */
         layers?: Array<string> /* An array of style layer IDs for the query to inspect. Only features 
                within these layers will be returned. If this parameter is undefined,
                all layers will be checked. */
     }
   ) {

This actually looks more palatable than I expected when I started typing, but I still think that in cases like this, it could be more readable to have the long, detailed parameter documentation up in the main function comment. (Admittedly, I'm not sure there's a good way to document the members of options in the main function comment -- maybe https://github.com/tmcw/dx-spec/issues/21#issuecomment-346910796.)

anandthakker avatar Nov 29 '17 13:11 anandthakker