comma-dangle doesn't report functions when using string option

[jwilsson]

Tell us about your environment

• ESLint Version: 6.1.0
• Node Version: 12.7.0
• npm Version: 6.10.2

What parser (default, Babel-ESLint, etc.) are you using?
default

Please show your full configuration:

Configuration

{
    "comma-dangle": ["error", "always"]
}

What did you do? Please include the actual source code causing the issue, as well as the command that you used to run ESLint.

foo(a)

What did you expect to happen?
I expected it to warn about the missing trailing comma after a. This is true no matter which string option I'm using. I saw that comma-dangle changed in 6.0.0 so I'm guessing it has something to do with that.

What actually happened? Please include the actual, raw output from ESLint.
No errors were reported.

Are you willing to submit a pull request to fix this bug?
Yes

[kaicataldo]

Thanks for the bug report. It looks like this behavior was changed in 6.0.0. I'm leaving the bug label on here because it looks like the documentation change doesn't currently match the actual behavior.

[eslint-deprecated]

Unfortunately, it looks like there wasn't enough interest from the team
or community to implement this change. While we wish we'd be able to
accommodate everyone's requests, we do need to prioritize. We've found
that issues failing to reach accepted status after 21 days tend to
never be accepted, and as such, we close those issues.
This doesn't mean the idea isn't interesting or useful, just that it's
not something the team can commit to.

Thanks for contributing to ESLint and we appreciate your understanding.

[jwilsson]

I'd still be happy to submit a PR fixing either the behaviour or if it's just a docs issue. I just need someone with a bit more insight into what the expected behaviour should be :)

[kaicataldo]

Maybe @aladdin-add can clarify this for us. It looks like maybe this line should be changed to match the change in this PR.

[kaicataldo]

Adding "accepted" since either the documentation or the code should be updated to match.

[yeonjuan]

I Fix it #12462. @kaicataldo Is it a too late?

[mojavelinux]

The change for this issue broke my eslint config. I use the following config:

"comma-dangle": ["error", "always-multiline"],

Now it reports that I'm missing a trailing comma for every function. Do I need to update my configuration to retain the previous rule?

[mdjermanovic]

@mojavelinux yes, the configuration should be something like this:

"comma-dangle": ["error", {
    "arrays": "always-multiline",
    "objects": "always-multiline",
    "imports": "always-multiline",
    "exports": "always-multiline",
    "functions": "ignore" // or "never" if you want to disallow
}]

[mojavelinux]

Wow, that's a pretty big breaking change. I can't believe that was accepted in a minor release. I've verified the suggestion works, so I don't have a problem updating my configuration. But I still don't see why the original configuration cannot work. The documentation seems to suggest it will still work: https://eslint.org/docs/rules/comma-dangle So this is either a bug or a semantic versioning mishap. (Just so it's clear, I'm not upset. I'm just pointing out that others might be).

[platinumazure]

I would be okay with reverting the change in semver-patch and deferring to semver-major, personally. I don't think this level of breakage was intended (at least, I wasn't intending it).

Probably best to add this to the TSC's agenda.

@mojavelinux Could you please open a new issue, so we can track this more easily? Thanks!

[kaicataldo]

It's unfortunate that it causes a lot more errors, but I do think this was the intended behavior and is a bug that should be fixed.

[mojavelinux]

It's intended behavior that specifying always-multiline applies to single-line functions? I'd like to understand the logic you're using to make that leap. If it's that the comma-dangle configuration should be specified as an object map, that goes directly against what is documented.

"This rule has a string option or an object option:" https://eslint.org/docs/rules/comma-dangle#options

In fact, the very first example shows the string option.

You can try to bend the truth, but the reality is that this change was not consistent with semantic versioning.

[mojavelinux]

@platinumazure Having thought about this more, it's beyond clear to me that this change needs to be reverted and deferred to the next major release.

[kaicataldo]

@mojavelinux Please try to be civil. While I appreciate your input, I don't think calling anyone a liar is particularly helpful or constructive in this situation. We're all doing our best to make the project the best it can be. Regarding semantic versioning, ESLint defines its semantic versioning policy here. If you have any questions or concerns about the policy, please feel free to make a new issue or drop by our Gitter chat.

Now, let's get back to the matter at hand.

It's intended behavior that specifying always-multiline applies to single-line functions?

It wasn't clear that this was what you were reporting, but if this is the behavior you're seeing then that does sound like a bug to me. I don't think that means we should necessarily revert the entire change, but we should definitely fix any unintended behavior. It looks like there are tests for this case here, so it would be helpful to to fix any edge cases that were missed.

Can you make a new issue with the specific problem you're seeing, filling out the requested template? That'll help us triage the problem much quicker. Thanks.

[mojavelinux]

I assure you I am intending to be civil. (As I said before, I'm not mad or yelling. I'm just trying to be very clear). I never said "lair". You have put words in my mouth. I asked to clarify the leap of logic, because I'm genuinely trying to understand.

I know you're doing your best and I am not saying otherwise. What I'm saying is that the documentation says that it's okay to specify the option as a string. The policy you linked to does not leave room for classifying that as a bug.

It wasn't clear that this was what you were reporting, but if this is the behavior you're seeing then that does sound like a bug to me.

Yes, that is what I'm reporting. Perhaps that makes it more understandable why I'm trying to sound the breaking change bells.

If I specify the following configuration:

"comma-dangle": ["error", "always-multiline"],

Then I get the following warnings for single-line function calls:

250:121  error  Missing trailing comma  comma-dangle

To fix it, I had to switch the second argument to a object/map:

"comma-dangle": ["error", {
  "arrays": "always-multiline",
  "objects": "always-multiline",
  "imports": "always-multiline",
  "exports": "always-multiline"
}],

If that's the direction you're going, I'm fine with that. But if you want to be consistent with your policy, that change should happen in a major version. I don't see this as a controversial request. (And it is just a request).

[kaicataldo]

Please make a new issue, filling out the requested bug template. We've noticed that ongoing discussions on closed issues tend to get overlooked, and it's best to make a new issue for visibility (particularly since this seems to be a bug caused by the fix this issue was reporting, but not this issue itself). Thanks.

[mojavelinux]

Per the policy, I would say it's this reason in the major version policy that applies here:

• A new option to an existing rule that results in ESLint reporting more errors by default.

Though that's a bit fuzzy because we're not really dealing with an existing rule.

If a case were made it violated a policy rule, it might be this one in the minor version policy:

• A new option to an existing rule that does not result in ESLint reporting more errors by default.

But this is kind of a gray area because it's kind of about removing an option format (if that is the direction that is intended). So I defer to your leadership on that.

[mojavelinux]

I don't think that means we should necessarily revert the entire change, but we should definitely fix any unintended behavior.

Agreed. That's what I meant to suggest.

[kaicataldo]

Great, sounds like we're in agreement.

[mojavelinux]

Then I get the following warnings for single-line function calls:

Doh! My mistake. (Foot in mouth). I meant to say multi-line function calls. Hmm, now I see the logic. always-multiline is just catching functions now.

So what we are talking about is making the single option of "always-multiline" also apply to functions.

I stand corrected that this is incorrect behavior. So even though it changed my perception of what the default should be, it is properly inheriting to all cases covered by this rule.

In the end, I guess I was the one who needed to rethink my logic ;) Sorry about that. And thanks for the discourse.

[mojavelinux]

I would like to restate how much I do appreciate the value this project brings. Seriously, all debate comes from a place of ❤️ and respect.

[mojavelinux]

After reflecting on my communication a bit more, I realize I shouldn't have said "bend the truth". I was attempting to use a figure of speech, but it misfired. I should have said "help me understand how you are arriving at the conclusion that this is a bug". Which I now do understand ;) I will work to improve my dialog in the future. Thanks again for your patience.