Rule Proposal: line-comment-position

[mysticatea]

From validateCommentPosition.

This rule will enforce the position of line comments.

{
    "line-comment-position": ["error", "avobe" or "beside"],
    // or
    "line-comment-position": [
        "error",
        {
            "position": "avobe" or "beside",
            "ignorePattern": "^\s*(?:eslint|jshint|jslint|istanbul|global|exported|jscs|falls?\s?through)"
        }
    ]
}

• position
  • "above" (default) - Requires to locate comments above codes.
  • "beside" - Requires to locate comments beside codes.
• ignorePattern (default is "^\s*(?:eslint|jshint|jslint|istanbul|global|exported|jscs|falls?\s?through)") - If the comment content matches the given regexp pattern, this rule ignore the comment.

[alberto]

What's the point of the any option? Isn't that the same as disabling the rule?

[mysticatea]

any option exists to disable either line comments or block comments via overrides option.
For example:

{
    "comment-position": {
        "position": "beside",
        "overrides": {
            "block": "any"
        }
    }]
}

[alberto]

Ok, so it would be an option just for the overrides, not for position.

If we want to extend the original rule to also check for block comments, I would propose the following schema (syntax errors aside):

{
    enum: ["above", "beside"]
},
{
    type: "object",
    properties: {
        block: {
            enum: ["above", "beside"]
        },
        line: {
            enum: ["above", "beside"]
        }
    },
    additionalProperties: false
}

[mysticatea]

Sounds good to me, except ignoreWord option is missing.
How about?

{
    enum: ["above", "beside"]
},
{
    type: "object",
    properties: {
        block: {
            anyOf: [
                {
                    enum: ["above", "beside"]
                },
                {
                    type: "object",
                    properties: {
                        position: {
                            enum: ["above", "beside"]
                        },
                        ignoreWords: {
                            type: "array",
                            items: {type: "string"},
                            uniqueItems: true
                        }
                    },
                    additionalProperties: false
                }
            ]
        },
        line: {
            anyOf: [
                {
                    enum: ["above", "beside"]
                },
                {
                    type: "object",
                    properties: {
                        position: {
                            enum: ["above", "beside"]
                        },
                        ignoreWords: {
                            type: "array",
                            items: {type: "string"},
                            uniqueItems: true
                        }
                    },
                    additionalProperties: false
                }
            ]
        }
    },
    additionalProperties: false
}

[platinumazure]

Do we need to consider having ignoreWords allow regexes? no-fallthrough and some other rules have been configured to allow regexes instead of strings.

[alberto]

Do you think ignoreWord is needed at the comment type level or should it be global?

[mysticatea]

Because /*eslint foo:2*/ is valid but //eslint foo:2 is invalid (no effect).
But it may not be important.

[mysticatea]

Regexp support sounds good to me.

[mysticatea]

Actually, original JSCS rule ignores block comments.
So this can change to line-comment-position and remove overrides and "any", also.

[alberto]

Yeah, I think it's better to limit ourselves to what the original rule does.

[mysticatea]

I updated this proposal by the result of discussion.

[alberto]

I would use ignorePattern instead of ignoreWords

[mysticatea]

I updated it.

[alberto]

@eslint/eslint-team I can champion this one if it gets accepted

[mikesherov]

👍

[nzakas]

👍

[BYK]

👍

[platinumazure]

For what it's worth, our default comment pattern to relax no-fallthrough is falls?\s?through, so I think we should change the default ignorePattern value to match that.

[mysticatea]

@platinumazure thank you! I updated it.

[SpadeAceman]

Does the acceptance of this rule this mean that the no-inline-comments rule would be deprecated?

[platinumazure]

@SpadeAceman Not sure why that rule should be deprecated-- it's a valid use case which is orthogonal to the use cases in this proposal.

[SpadeAceman]

I've taken a closer look, and it still seems to me that the functionality of no-inline-comments is entirely covered by specifying "position":"above" with this new rule. What am I missing that's different?

[platinumazure]

We have deprecated old rules and superseded them with new rules in the past. We could theoretically do the same here, if deemed worthwhile. That said, the bar is pretty high.

[alberto]

@SpadeAceman there is some overlap, but this doesn't cover every use case. This rule only deals with line comments, while no-inline-comments also checks block comments.

[alberto]

I'm working on this and I have one question. If a custom ignorePattern is specified, should we add it to the default one or replace it? The JSCS rule adds it. Otherwise we would report on eslint comments, for example, unless users add it to their custom pattern.

[mysticatea]

Replacing was my intention because I thought good that there is a way to opt the default ignored patterns out.
But I think just adding is good as well.

I don't have strong opinions here.

[platinumazure]

I like replacing on principle (always a bad idea to lock a developer into a certain pattern). However it would be convenient if we provided a way to add patterns. Could we do something like "ignoreDefaultPatterns: true" to allow users to add patterns while simultaneously continuing to ignore the patterns ignored by default? (Pick a better name for the option, of course, but you get the idea.)