Update: auto-fix for comma-style (fixes #6941)

[gyandeeps]

What issue does this pull request address?

Introduce auto-fix for comma-style rule.

What changes did you make? (Give an overview)

Fix the position of comma.

Is there anything you'd like reviewers to focus on?

This is just a proof of concept PR.

UPDATE:

Its ready for review and merge.

[eslintbot]

LGTM

[mention-bot]

@gyandeeps, thanks for your PR! By analyzing the annotation information on this pull request, we identified @vitorbal, @kaelzhang and @evansimmons-ck to be potential reviewers

[eslintbot]

LGTM

[platinumazure]

I think we usually want "code" or "whitespace" here. Not sure which this is though!

[ilyavolodin]

This would be "code". "whitespace" is basically just for spacing fixes.

[eslintbot]

LGTM

[eslintbot]

LGTM

[mysticatea]

This fixing seems to be for style === "last".
If style === "first", probably it should be fixer.removeRange([commaToken.range[1], currentItemToken.range[0]]).

[mysticatea]

I think this rule should not remove comments.
We often write comments between properties.

let obj = {
    foo: 1,  // a comment
    bar: 2,  // another comment

    /**
     * a doc comment
     */
    method: function() {
    },
}

Maybe...:

const text =
    sourceCode.text.slice(prevToken.range[1], commaToken.range[0]) +
    sourceCode.text.slice(commaToken.range[1], currentToken.range[0])
const range = [prevToken.range[1], currentToken.range[0]]

// first → last
fixer.replaceTextRange(range, `${text},`)

// last → first
fixer.replaceTextRange(range, `,${text}`)

[gyandeeps]

As always @mysticatea to the rescue. Excellent idea. So far looks good.

[eslintbot]

LGTM

[mysticatea]

This is dangerous.

let obj = {
    a // a comment
    ,
    b
}

I think good to handle the same way as others for lonely comma's cases also. I.e. getFixerFunction(style, previousItemToken, commaToken, currentItemToken)

Or maybe it should not fix the lonely commas.

[gyandeeps]

@mysticatea Sorry but I din't quite get what you trying to say here.

I have a unit test covering the above use case, and the fixed code would be

let obj = {
    a, // a comment
    b
}

[mysticatea]

Oops, sorry, it was my misunderstand.
I sometimes forget that String.prototype.replace replaces only the first matched part.

[mysticatea]

LGTM, thank you!

[nzakas]

Just want to double check that we are only doing fixes that are 100% safe with this now?

[mysticatea]

@nzakas I think so. The logic is:

let obj = { foo: 1 // comment\n  /**\n   * comment\n   */\n,   bar: 2 }
                  └┬──────────────────────────────────────────┘
                   └ Part A - between the end of the previous property and the current property.

let obj = { foo: 1 // comment\n  /**\n   * comment\n   */\n,   bar: 2 }
                  └┬──────────────────────────────────────┘
                   └ Part B - between the end of the previous property and the comma.

let obj = { foo: 1 // comment\n  /**\n   * comment\n   */\n,   bar: 2 }
                                                            └┬┘
        Part C - between the comma and the current property. ┘

Then, it replaces A (= B + "," + C) by "," + B + C or B + C + ",".
It will move the comma exactly.

In addition, in "first" cases, comma-spacing and indent rules will tweak the code in the next passes of autofix.

[ilyavolodin]

I think this is ready to be merged, but I'm not 100% if all of the comments have been resolved. @mysticatea could you confirm and merge if you think it's good to go?

[mysticatea]

Sure, LGTM.