Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Rule Proposal: comment-capitalizing #6055

Closed
mysticatea opened this issue May 3, 2016 · 18 comments
Closed

Rule Proposal: comment-capitalizing #6055

mysticatea opened this issue May 3, 2016 · 18 comments
Assignees
Labels
accepted There is consensus among the team that this change meets the criteria for inclusion archived due to age This issue has been archived; please open a new issue for any further discussion feature This change adds a new feature to ESLint rule Relates to ESLint's core rules

Comments

@mysticatea
Copy link
Member

mysticatea commented May 3, 2016

From requireCapitalizedComments and disallowCapitalizedComments.

This rule will enforce which the first character is an upper case or a lower case.

{
    "comment-capitalizing": ["error", "always" or "never"],
    // or
    "comment-capitalizing": [
        "error",
        {
            "capitalize": "always" or "never",
            "markers": ["*"],
            "ignorePattern": "^\s*(?:eslint|jshint|jslint|istanbul|global|exported|jscs|falls?\s?through)",
            "ignoreInlineComments": false,
            "overrides": {
                "block": null,  // e.g. "always", {capitalize: "always", markers: ["!"]}, ...
                "line": null
            }
        }
    ]
}
  • "always" (default) - Requires the first alphabetical character of a comment to be uppercase, unless it is part of a multi-line textblock.
  • "never" - Requires the first alphabetical character of a comment to be lowercase.
  • markers (default is ["*"]) - If the head of comments matched to markers, this rule cut the matched part off, then checks the comment capitalizing. (this is similar to the markers option of spaced-comment rule)
  • ignorePattern (default is "^\s*(?:eslint|jshint|jslint|istanbul|global|exported|jscs|falls?\s?through)") - If the comment content (except the part that markers option has matched) matches the given regexp pattern, this rule ignore the comment.
  • ignoreInlineComments (default is false) - If this is true, this rule will be applied to only comments which are at the head of each line. (from requireCapitalizedComments's inlined option)
  • overrides - People can override setting for each line or block comment.
@mysticatea mysticatea added rule Relates to ESLint's core rules feature This change adds a new feature to ESLint evaluating The team will evaluate this issue to decide whether it meets the criteria for inclusion labels May 3, 2016
@mysticatea mysticatea added this to the JSCS Compatibility milestone May 3, 2016
@akuznetsov-gridgain
Copy link

akuznetsov-gridgain commented Jun 1, 2016

+1 From me. Also it is possible to add to this rule a check for "." at the end of comment?

@platinumazure
Copy link
Member

platinumazure commented Aug 2, 2016

@akuznetsov-gridgain I would suggest creating a separate issue for your request. (In your issue, please note if there is a JSCS equivalent or not, so we know whether or not we should treat the new issue as JSCS compatibility related or not.)

Also, I'd like to try implementing this (goal is to do so by this weekend). Assigning to myself for now. Never mind, issue is not accepted. I will champion.

@platinumazure platinumazure self-assigned this Aug 2, 2016
@platinumazure
Copy link
Member

@mysticatea Any idea how to handle non-ASCII characters? What about non-letters, should those trigger the rule under any circumstances or not?

@platinumazure
Copy link
Member

platinumazure commented Aug 2, 2016

@eslint/eslint-tsc I don't know if we still want to expedited-accept JSCS compat issues or if we should go through the usual new rule process. I have a couple of open questions about non-ASCII and non-letter characters, but this otherwise seems like an actionable proposal and I think this should be accepted.

If we are not expediting JSCS compat issues, I will champion this rule.

@alberto
Copy link
Member

alberto commented Aug 3, 2016

I think default ignoreWords should always be applied. I don't think this should even be an option like in #6077

@nzakas
Copy link
Member

nzakas commented Aug 3, 2016

@platinumazure we should try to follow the regular process here, so you can champion it.

@nzakas nzakas added accepted There is consensus among the team that this change meets the criteria for inclusion and removed evaluating The team will evaluate this issue to decide whether it meets the criteria for inclusion labels Aug 3, 2016
@mysticatea
Copy link
Member Author

@platinumazure In Japanese, we don't have upper/lower cases. This rule simply should ignore Japanese letter. (though I'm not sure about other languages). I think text[0] === text[0].toLocaleUpperCase() is good. This check would become true if the text starts with an uppercase letter or a case-less letter.

@mysticatea
Copy link
Member Author

Ah... This rule should skip markers. It may need markers option, or simply it would skip symbol letter at the head of comments.

@platinumazure
Copy link
Member

platinumazure commented Aug 10, 2016

Okay, by my count of 👍's on my champion comment, I think this is ready to roll. I'll try to get this out within the next week or so. (I'm moving house on Friday, though, so my life will be chaotic for the next few days.)

@mysticatea Could I ask you to edit the first post with the markers option? I assume it's at the same level as capitalize/ignoreWords/onlyHeader but I want to be absolutely sure 😄

@platinumazure
Copy link
Member

platinumazure commented Aug 14, 2016

Started working on this. It'll be another day or two before I have a branch up, but I've already got some questions:

  • onlyHeader/inline option: I looked at the JSCS page and I'm wondering if inline actually means a block comment embedded in a line of code? (For example, function doFoo(a, /* bar */ b) {}) Can anyone provide some insight here?
  • ignoreWords: Should the default ignoreWords be always ignored by default, or should it be possible to remove those defaults by specifying an array without them? (And if the latter, do we really want to allow that for "eslint" in particular, since it could mean all inline config comments would be reported?)
  • What about JSDoc comments? Should those be factored in here, ignored entirely, or should we add an option to include/exclude them from this rule? (Should the valid-jsdoc rule be separately enhanced to validate initial capitalization of various parts of JSDoc comments? To me that seems like the best approach for JSDoc. Either way, I still need to know what to do with JSDoc comments in this rule.)

@nzakas
Copy link
Member

nzakas commented Aug 15, 2016

Inline - yes, that's what it means.

IgnoreWords - I think we should always ignore the four defaults and not allow them to be unignored (so they shouldn't be part of configuration, only internal).

JSDoc comments are treated the same as any other.

@nzakas
Copy link
Member

nzakas commented Aug 15, 2016

@markelog can you double check what I said?

@platinumazure I think the JSCS unit tests are a good resource when questions come up: https://github.com/jscs-dev/node-jscs/blob/master/test/specs/rules/require-capitalized-comments.js

@platinumazure
Copy link
Member

@nzakas I'd like to propose a modification to @mysticatea's proposal in that case:

  • onlyHeader option to be renamed to ignoreInlineComments, with the values still being boolean, default false, and having the same meaning as the proposal.

@mysticatea
Copy link
Member Author

@platinumazure I'm sorry for my slow response. I have missed notification.

I edited the first post by some suggestions.

@markelog
Copy link
Member

Inline - yes, that's what it means.

Yeah, it is even mentioned in the valid/invalid rule examples.

On the side note, right now, I would just implement JSCS options and consider implementing others if there would be sufficient enough amount of users to want them

@platinumazure
Copy link
Member

Started work on this. Documentation is pretty much done (well, unless we want more examples of different combinations of options). Rule logic seems to be done, now I'm just writing test cases and squashing bugs. I'll get a branch up this weekend once I've made some more progress.

@eslint/eslint-tsc Strong opinions on whether to include markers at this point, based on @markelog's comment here?

@platinumazure
Copy link
Member

I've been beavering away at this for the last couple of days, and I think I'll have a PR (re)opened tomorrow.

Decisions I've made unilaterally (subject to discussion in the PR):

  • Not adding markers (unnecessary for JSCS compatibility)
  • Ensuring comments that begin with URLs don't get reported

Still left:

  • Implement "textblock" support
    • "Textblock" is the term JSCS used for consecutive comments, be they line or block. The rule will not report the second or later comment in a "textblock" as long as the first comment follows the rule.
    • To me, this is kind of weird default behavior (users can use block comments for multiple lines usually), especially with consecutive block comments, so I'm going to put this behind an option.

The "textblock" bit should be the last thing I need to do before it's ready for review.

@platinumazure
Copy link
Member

New PR is open: #7415.

@nzakas nzakas closed this as completed in ad56694 Nov 25, 2016
@eslint-deprecated eslint-deprecated bot locked and limited conversation to collaborators Feb 6, 2018
@eslint-deprecated eslint-deprecated bot added the archived due to age This issue has been archived; please open a new issue for any further discussion label Feb 6, 2018
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
accepted There is consensus among the team that this change meets the criteria for inclusion archived due to age This issue has been archived; please open a new issue for any further discussion feature This change adds a new feature to ESLint rule Relates to ESLint's core rules
Projects
No open projects
Development

No branches or pull requests

6 participants