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

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

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 bug ESLint is working incorrectly Hacktoberfest Recommended issue for those participating in Hacktoberfest https://hacktoberfest.digitalocean.com/ help wanted The team would welcome a contribution from the community for this issue rule Relates to ESLint's core rules

Comments

@jwilsson
Copy link
Contributor

jwilsson commented Aug 3, 2019

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

@jwilsson jwilsson added bug ESLint is working incorrectly triage An ESLint team member will look at this issue soon labels Aug 3, 2019
@kaicataldo kaicataldo added evaluating The team will evaluate this issue to decide whether it meets the criteria for inclusion rule Relates to ESLint's core rules and removed triage An ESLint team member will look at this issue soon labels Aug 3, 2019
@kaicataldo
Copy link
Member

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 eslint-deprecated bot added the auto closed The bot closed this issue label Sep 3, 2019
@eslint-deprecated
Copy link

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
Copy link
Contributor Author

jwilsson commented Sep 4, 2019

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 kaicataldo removed the auto closed The bot closed this issue label Sep 4, 2019
@kaicataldo
Copy link
Member

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 kaicataldo reopened this Sep 4, 2019
@kaicataldo kaicataldo 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 Sep 4, 2019
@kaicataldo
Copy link
Member

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

@kaicataldo kaicataldo added help wanted The team would welcome a contribution from the community for this issue Hacktoberfest Recommended issue for those participating in Hacktoberfest https://hacktoberfest.digitalocean.com/ labels Sep 29, 2019
@yeonjuan
Copy link
Member

yeonjuan commented Oct 22, 2019

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

@mojavelinux
Copy link

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
Copy link
Member

@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
Copy link

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
Copy link
Member

platinumazure commented Nov 2, 2019

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
Copy link
Member

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
Copy link

mojavelinux commented Nov 4, 2019

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
Copy link

@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
Copy link
Member

@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
Copy link

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
Copy link
Member

kaicataldo commented Nov 4, 2019

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
Copy link

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
Copy link

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
Copy link
Member

Great, sounds like we're in agreement.

@mojavelinux
Copy link

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
Copy link

mojavelinux commented Nov 4, 2019

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
Copy link

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.

@eslint-deprecated eslint-deprecated bot locked and limited conversation to collaborators Apr 22, 2020
@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 Apr 22, 2020
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.