Update: Enable function string option in comma-dangle (fixes #12058)

[yeonjuan]

What is the purpose of this pull request? (put an "X" next to item)

[ ] Documentation update
[X] Bug fix (template)
[ ] New rule (template)
[ ] Changes an existing rule (template)
[ ] Add autofixing to a rule
[ ] Add a CLI option
[ ] Add something to the core
[ ] Other, please explain:

What changes did you make? (Give an overview)

• Enable function string option in comma-dangle.

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

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

[platinumazure]

Are there any tests which show that functions are ignored by default when ecmaVersion < 8?

I didn't see any new tests along those lines and a quick skim of the existing valid test cases didn't show me any such tests (but I could have missed some).

[yeonjuan]

@platinumazure You are right. There were Insufficient test cases. I add more test cases. Thanks:)

[platinumazure]

Thanks for adding those test cases!

Looks good to me, but I would like more team members to review as well.

[mdjermanovic]

Thanks for the correct change and comprehensive tests! I left just one minor note for an additional test.

I think it's correct to treat this as a bug fix with more warnings for a semver-minor, because this behavior
matches the documentation, and it was announced in migrating to 6.0.0:

In ESLint v6, function commas are treated the same way as other types of trailing commas.
...
To restore the previous behavior of a string option like "always-multiline", replace "never" with "always-multiline" in the example above.

Also, it's possible to reconfigure the rule if the new warnings are not desired (might be good to put a note in the release blog post?).

[yeonjuan]

@mdjermanovic Thanks. I added a test case(ecmaVersion >8)

[mdjermanovic]

@mdjermanovic Thanks. I added a test case(ecmaVersion >8)

Apologies, I meant an additional test in invalid but mistakenly marked code in valid :-(

The test case you added is anyway useful and should stay.

If you can it would be nice to add one more test with ecmaVersion: 9 in invalid. If not, I'm fine with merging this PR as is. Sorry for the mistake!

[yeonjuan]

@mdjermanovic No problem! I added one test case in invalid for ecmaVersion 9

[mdjermanovic]

Thanks for the test, and sorry again. Everything LGTM, thanks for contributing!

[platinumazure]

Merged-- thanks @yeonjuan for contributing!