1220796 - ESLint rule to warn about TODO/FIXME/XXX comments without a bug number

Status: NEW

(DevTools :: General, defect)

Description

[Patrick Brosset <:pbro>]

The ESLint rule "no-warning-comments" seems useful to warn about TODO/FIXME/XXX comments but as discussed in bug 1220601, we don't want ESLint to make those appear as code warnings where, in fact, we do want to write them and they are useful.

A more useful rule is: warn about TODO/FIXME/XXX/... comments that *do not* have an associated bug number.

The rule should be heavily based on the "no-warning-comments" rule [1] but on top of this should match 'Bug N' or 'https://bugzilla.mozilla.org/show_bug.cgi?id=N'

[1] https://github.com/eslint/eslint/blob/master/lib/rules/no-warning-comments.js

Comment 1

[J. Ryan Stinnett [:jryans] (Use needinfo, replies may be slow)]

Another format to check for is "https://bugzil.la/N".

Comment 2

[Patrick Brosset <:pbro>]

Attachment: Bug_1220796_-_ESLint_rule_that_warns_about_TODO_co.diff

Spent a little time hacking on the eslint todo warning rule to add this functionality to it.
This seems to work well.

One thing though, and I don't know how practical this is to fix with eslint: eslint considers several line comments (//) that follow each others to be separate line comments, not a single multi-line comment (/** */).
This means that this is parsed as 3 separate line comments:

// TODO: we need to fix
// this big problem in
// bug 12345.

and therefore the rule will warn about the missing bug number in the first line.

Either we spend more time fixing this (not sure how), or we make a habit of putting the bug number on the first line of these blocks (or use multi-line comments /** */).

Comment 3

[Patrick Brosset <:pbro>]

AST nodes in eslint contain location information (line/col numbers), so it shouldn't be too hard to mark a TODO line comment as valid after the fact, if we find that it's followed by other line comments that contain bug numbers. But that means having a catch-all visitor registered for all code node types to know when a block of multiple line-comments end. Or something like that ...

Comment 4

[Mike Ratcliffe [:miker] [:mratcliffe] [:mikeratcliffe]]

I think you can walk the nodes to get all the text for // comments... use console.dir(node) to find the function to use.

Comment 5

[Mike Ratcliffe [:miker] [:mratcliffe] [:mikeratcliffe]]

(In reply to Michael Ratcliffe [:miker] [:mratcliffe] from comment #4)
> I think you can walk the nodes to get all the text for // comments... use
> console.dir(node) to find the function to use.

I was wrong and it is way more complex than I expected... you can use this helper function though (Please add it to helpers.js):
```
  /**
   * Get the comments from a LineComment or BlockComment event.
   *
   * @param  {ASTNode} node
   *         For LineComments the first comment node.
   *         For BlockComments there is only one node.
   *
   * @return {String}
   *         Comment text.
   */
  function getCommentText(node) {
    if (node.type == "Line") {
      if (node.value.length === 0) {
        return;
      }

      var text = context.getSourceCode().text;
      var prevText = text.substr(0, node.range[0] - 1).trim();
      var trailing = /\n\s*\/\/.*$/.test(prevText);

      if (trailing) {
        return;
      }

      var comment = "";

      text = text.substr(node.range[0]).trim();

      while (text.startsWith("//")) {
        text = text.substr(2);
        var line = text.match(/^(.*?)\n/)[1];

        if (line) {
          comment += line;
        }

        text = text.substr(line.length + 1).trim();
      }

      return comment.trim();
    }

    return node.value || "";
  }
```

Comment 6

[Gerald Norfork]

Attachment: same problem

Thanks from cherlilenew1981.