Enforce capitalization of the first letter of a comment (capitalized-comments)

[computersarecool]

I am wondering if this project favors sentence case or all lower case for comments?
i.e.
// This is my comment

or

// this is my comment

Or if it even matters.

[feross]

I'm not entirely consistent in my own code, though I think proper capitalization is better. There's no rule for this in ESLint, but there's a proposal to add it. When that gets added, I'll post an issue to consider it.

[feross]

ESLint now has a capitalized-comments rule.

I propose we enable it, as follows:

"capitalized-comments": [
        "error",
        "always",
        {
            "ignoreInlineComments": true,
            "ignoreConsecutiveComments": true
        }
    ]

This rule is also automatically fixable.

[dcousens]

Woo!
I hate having to think about this junk 👍

[timoxley]

@feross not disagreeing but why ignoreInlineComments?

[feross]

@timoxley Maybe you're right. I'll test the ecosystem impact of ignoreInlineComments enabled vs. disabled and report back here before merging this.

My thought was just that there could be reasons to do:

function foo (/* host, port */) {
  var host = arguments[0]
  var port = arguments[1]
}

I do this kind of thing in feross/buffer sometimes since there's so many argument orderings to check for in some of the node core stuff. But this is not a big deal to me.

[dcousens]

You typically see this for those who use the type systems analysis tools which pick up on inline comments for type declaration in standard JS code. IIRC

[feross]

@dcousens If we want to, we can explicitly allow certain uncapitalized words to start a comment without triggering this error. The rule supports exceptions. But a blanket exemption on inline comments is certainly simpler and won't need constant updating.

[Flet]

The inline eslint pragmas need to all be lowercase:

/* Eslint-env mocha */   // Does not work!
/* eslint-env mocha */   // Works!

I wonder how many other transpilers/helpers would break because of forced uppercase.
Flow? JSDoc?

I tending to be thumbs down on this just because the value may not be worth the additional breakage (or the additional maintenance of a whitelist).

[yoshuawuyts]

I don't think we should - I don't feel this particular rule would be useful, and might, in fact, become frustrating. With stuff like code comments, function names and what not there's simply too many edge cases

…

On Wed, Jan 11, 2017 at 5:55 PM Dan Flettre ***@***.***> wrote: The inline eslint pragmas need to all be lowercase: /* Eslint-env mocha */ // Does not work!/* eslint-env mocha */ // Works! I wonder how many other transpilers/helpers would break because of forced uppercase. Flow? JSDoc? I tending to thumbs down on this just because the value may not be worth the additional breakage (or the additional maintenance of a whitelist). — You are receiving this because you are subscribed to this thread. Reply to this email directly, view it on GitHub <#682 (comment)>, or mute the thread <https://github.com/notifications/unsubscribe-auth/ACWlerpT-XhPG3RABtA1JNit7X5DsGOXks5rRQlxgaJpZM4KvDVD> .

[feross]

@Flet All eslint pragmas are special-cased by the rule already.

Note that the following words are always ignored by this rule: ["jscs", "jshint", "eslint", "istanbul", "global", "globals", "exported"]

JSDoc is not affected because those declarations all start with a symbol (@) and comments are allowed to start with symbols and unicode characters. It's only if they are normal letters that they must be capitalized.

@yoshuawuyts Personally, I think this is a pretty well-thought-out rule: http://eslint.org/docs/rules/capitalized-comments I'd like to try shipping it in the standard v9 beta if there isn't strong objection so we can see how it works in practice.

[timoxley]

Note that the following words are always ignored by this rule: ["jscs", "jshint", "eslint", "istanbul", "global", "globals", "exported"]

Yeah I'm a little concerned about accidentally breaking non-eslint/less popular/custom pragmas. Would be interested to see ecosystem impact.

[jprichardson]

Wouldn't commented out code be affected as well? If so, this seems pretty overreaching.

[feross]

@jprichardson That's a good point that I hadn't considered. This rule basically prevents commented code from being committed.

For example, this would error:

function foo (x) {
  // if (x === 1) return 10
  return x
}

In one way, it would be nice to prevent commented code from ending up in a codebase. But sometimes this is not avoidable, so I think I agree with @jprichardson now: this is a bit overreaching.

@timoxley's point is good too.

[jprichardson]

Yeah, I agree in most cases, leaving commented code in place is a bad idea, especially since tools like Git are so pervasive. However, there are legitimate reasons to have commented code, like config files. Here is a real example from my codebase:

export const EXODUS_SERVER = EXODUS_PRODUCTION_SERVER
// export const EXODUS_SERVER = EXODUS_STAGING_SERVER

EXODUS_PRODUCTION_SERVER and EXODUS_STAGING_SERVER are defined near the beginning of the file.

So now if I just implemented a new feature on the server, I can quickly uncomment the aforementioned block to this:

// export const EXODUS_SERVER = EXODUS_PRODUCTION_SERVER
export const EXODUS_SERVER = EXODUS_STAGING_SERVER

and then the whole app is using EXODUS_SERVER still, but its value is different. Then when I'm ready to go back to a production build, I can change. Now I realize that it may be better to configure the aforementioned based upon NODE_ENV, but that'd feel like unnecessary ceremony required by a linter.

I'm sure there's other cases that are equally as valid where this rule my impede.

[dcousens]

Now I realize that it may be better to configure the aforementioned based upon NODE_ENV

My thoughts exactly.
I'm trying to think of other cases for this, but ultimately, committed code has always been the wrong decision for me.

[jprichardson]

@jprichardson sounds like a better use-case of process.env

You just have missed this part 😛

Now I realize that it may be better to configure the aforementioned based upon NODE_ENV, but that'd feel like unnecessary ceremony required by a linter.

[dcousens]

@jprichardson I did, so I edited 😆 , you guys and your email notifications

[feross]

This rule has a big ecosystem impact:

1..422
# tests 422
# pass  259
# fail  163

To reduce the impact, I investigated allowing commented out code by using the ignorePattern option. We can ignore comments that start with a JS reserved word (do, if, in, for, let, etc.). Comments that start with non-ascii characters are automatically allowed ([, (, etc.)

So, if we only catch comments with real words that need to be capitalized, we get:

1..422
# tests 422
# pass  268
# fail  154

New rule is:

    "capitalized-comments": [
      "error",
      "always",
      {
        "ignorePattern": "do|if|in|for|let|new|try|var|case|else|enum|eval|null|this|true|void|with|await|break|catch|class|const|false|super|throw|while|yield|delete|export|import|public|return|static|switch|typeof|default|extends|finally|package|private|continue|debugger|function|arguments|interface|protected|implements|instanceof",
        "ignoreInlineComments": true,
        "ignoreConsecutiveComments": true
      }
    ],

This is automatically fixable, so it's a quick fix. But jury's out on whether we actually want to do this.

[feross]

For example, here are some diffs after running standard --fix:

The comments that start with variables could be wrapped in backticks like `err` so they don't need to be capitalized, but this might not be obvious to users.

[yoshuawuyts]

No please

…

On Thu, Feb 9, 2017, 02:36 Feross Aboukhadijeh ***@***.***> wrote: For example, here are some diffs after running standard --fix: [image: screen shot 2017-02-08 at 5 35 16 pm] <https://cloud.githubusercontent.com/assets/121766/22765421/0ecc8e72-ee25-11e6-87d3-65b96fd1ab6d.png> [image: screen shot 2017-02-08 at 5 35 28 pm] <https://cloud.githubusercontent.com/assets/121766/22765422/0ecf4612-ee25-11e6-8100-de3b35a87740.png> — You are receiving this because you were mentioned. Reply to this email directly, view it on GitHub <#682 (comment)>, or mute the thread <https://github.com/notifications/unsubscribe-auth/ACWlemDgdfp2g8B_ljW5oOn2jiiPmWBmks5ram2UgaJpZM4KvDVD> .

[feross]

Leaving open for more feedback, but yeah, I tend to agree with you @yoshuawuyts

[timoxley]

This has good intentions but my feeling is that the benefits are minimal and mostly aesthetic (i.e. does not reduce risk of any class of bugs) and yet the likelihood of it being a nuisance is high. Also don't like the risk of hard-to-detect logic corruption caused by uppercasing commented out source which is later uncommented.

[feross]

@dcousens thoughts?

[timoxley]

Along similar lines, maybe one could consider enforcing a particular formatting for multi-line jsdoc /** styled comments i.e. ensure jsdoc style comments conform to the/a particular jsdoc spec.

[dcousens]

Agreed with @timoxley, its junk to think about, but, its harmless by definition and IMHO, not worth the community impact at all.
I could not morally justify this refactor to someone paying the bills, haha.

[feross]

Alright, it's decided. 💀

[Flet]

This has been a great thread :)