enhance require-jsdoc to recognize function expressions

[vobu]

ESLint-version: 1.10.3, default parser

configuration:

"require-jsdoc":
                [
                        2,
                        {
                                "require":
                                {
                                        "FunctionDeclaration": true,
                                        "MethodDefinition": false,
                                        "ClassDeclaration": false
                                }
                        }
                ],

Running eslint (from a grunt task), I would expect this to cause an error:

module.exports.foo = function foo() {
    return 'bar';
}

Instead, it passes through fine.

So that's why I would propose an enhancement to the require-jsdoc rule to also recognise missing JSDoc blocks in functional expressions such as the above.

All of the below should pass through fine:

/**
* some description
* 
* @returns {string}
*/
module.exports.foo = function foo() {
    return 'bar';
}

/**
 * some description
 *
 * @returns {string}
 */
foo: function() {
    return 'bar';
}

[BYK]

I'm 👍 to the suggestion.

[nzakas]

This is trickier than it seems, which is why it is not done yet. :)

We can't just flag any function expression, because it might not be considered a method. For instance, should this be flagged?

var foo = function() {};

How about this?

somethingFunction({
    apply: function() {}
});

I'm not opposed to exploring this, I just think we need to be very specific about the patterns we should and should not flag because it's not as simple as requested.

[BYK]

@nzakas - I guess if we find a reliable way to detect in-place callback definitions which I think is possible I think all others should be documented. We may have sub-options, say for methods to allow the rule behave a bit more relaxed. Makes sense?

[qfox]

If I got it right, we should take a look at https://github.com/jscs-dev/jscs-jsdoc/blob/master/lib/rules/validate-jsdoc/enforce-existence.js with their options (since this rule was bundled into jscs package).

We got there 'exports' and 'paramless-procedures': jscs-dev/jscs-jsdoc#139.
'exports' stands for export some function(){} and module.exports = function(), and paramless-procedures for functions without params and return statements.

[nzakas]

@zxqfox nice! We should be able to reuse that detection logic. Can you list here (or point to docs) that explain the various options?

@BYK yes, I think if we can reuse some of what's in JSCS, we probably have a good shot.

[qfox]

Yeah, sure.

By default it reports any function without bound jsdoc-block except anonymous functions. It tries to find comment-block on the previous (or skip one empty line, except file start) with the same indent as statement with target function.

E.g.:

/**
 * File overview
 */

var fn = function() {
}

/* nothing here */

/**
 * Function description
 */

var fn = function() {
}

And next to it there are several filters:

• anonymous: enabled by default, describes functions without names (node.name.id or parentAssignmentStatement.left.id), except arrow functions (since they can't have a name) — https://github.com/jscs-dev/jscs-jsdoc/blob/master/lib/rules/validate-jsdoc/enforce-existence.js#L72
• arrow: for arrow functions, I think, this one needs rework.
• exports: for different exports mechanisms (like module.exports = function, and es6 exports) — https://github.com/jscs-dev/jscs-jsdoc/blob/master/lib/rules/validate-jsdoc/enforce-existence.js#L79
• expressions: for function inside different expression (like IIFE, var fn = function, { fn: function }, etc.), probably useful — https://github.com/jscs-dev/jscs-jsdoc/blob/master/lib/rules/validate-jsdoc/enforce-existence.js#L91
• 'paramless-procedures': for functions without arguments and return statements without value: https://github.com/jscs-dev/jscs-jsdoc/blob/master/lib/rules/validate-jsdoc/enforce-existence.js#L97

I'm not sure these are the best ones. But it's what we have in jscs-jsdoc atm.

[BYK]

@zxqfox - Wanna give this a shot? Otherwise I'll go ahead and try to implement it.

[qfox]

Not now :-( Go ahead!

[nzakas]

Marking as accepted because we need these changes for JSCS compat.

[doberkofler]

+1

[joelbarker2011]

Somewhat related is the situation of functions within objects. I expected the following to be flagged by the require-jsdoc option, but it wasn't:

/*
eslint "require-jsdoc": ["error", {
    "require": {
        "FunctionDeclaration": true,
        "MethodDefinition": true,
        "ClassDeclaration": true
    }
}]
*/

export default {
  foo: false,

  // these are not the docs you are looking for
  bar: function () {
    this.foo = true;
  },

  // sorry, but the docs are in another castle
  baz() {    // ES6-style method shortcut
    this.foo = false;
  },
};

[nzakas]

@zxqfox are you planning on addressing this?

[qfox]

Wow. Yeah, I'll try to give this a shot!

[mightypenguin]

Pinging this so people know there is still interest.
This would bring my documentation to 90+%.

All I care about is:
A)
var myfunc = function() {}

B)
var object = {
myfunc: function() {}
}

[chazmuzz]

+1 I'm looking for it to flag returning a named function so that it works with my AMD solution. Currently the following code is OK even though I have require-jsdoc:

define(function() {
  return function Foo() {
    // code
  }
})

However this similar code fails linting:

define(function() {
  function Foo() {
    // code
  }
  return Foo;
})

[sunjay]

The code I would like this to work for looks like this:

const Actions = {
  actions: new Set(),
  register(name) {
    ...
  },
};

Similar to what everyone else is presenting except instead of register: function(name) { I'm using register(name) { which is allowed in ES6.

[razbakov]

+1

[platinumazure]

Could we augment the rule to support selectors? That way, people could enforce documentation on whatever function node types they want to (including only ones that are part of assignments, or part of object expressions, or...).

[not-an-aardvark]

If I'm not mistaken, selectors wouldn't be an immediate solution to this problem because we would still have to match the JSDoc comment to the function expression itself. (This issue isn't asking for more configurability -- it's asking for functionality that isn't implemented yet.)

[kaicataldo]

I started looking at this, but it doesn't look like we actually decided on an implementation (it was accepted because it was needed for JSCS compatibility). My understanding is that we want options to enforce JSDocs on the following:

/*
 * JSDoc Block
 */
const foo = function() {}

const foo = {
  /*
   * JSDoc Block
   */
  bar: function() {},

  /*
   * JSDoc Block
   */
  baz() {}
}

Assuming my understanding is correct (let me know if it's not!), it seems like these would need to be two new options (turning them on by default would be breaking and could potentially cause a lot of errors).

@eslint/eslint-team Let's hash this out and I'll get to work!

[not-an-aardvark]

Could you clarify what the two options would do? I might be missing something, but I only see a need for one new option (to control whether function expressions are checked).

[kaicataldo]

Sorry that wasn't clear. After my initial reading, seemed like the two asks are to be able to require JSDocs for function expressions in assignments and object methods (both function expressions and object method shorthand syntax). I feel like it would be counter-intuitive to lump the object shorthand syntax in with a function expression option, but if everyone else is fine with that, I won't object.

More concerned with getting this implemented for JSCS compatibility than anything else!

[kaicataldo]

I'm going to implement this with a FunctionExpression option and we can decide later what we think.