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

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

Merged
merged 1 commit into from Oct 23, 2019

Conversation

@yeonjuan
Copy link
Member

yeonjuan commented Oct 19, 2019

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?

Copy link
Member

platinumazure left a comment

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

This comment has been minimized.

Copy link
Member Author

yeonjuan commented Oct 22, 2019

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

Copy link
Member

platinumazure left a comment

Thanks for adding those test cases!

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

Copy link
Member

mdjermanovic left a comment

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?).

tests/lib/rules/comma-dangle.js Show resolved Hide resolved
@yeonjuan

This comment has been minimized.

Copy link
Member Author

yeonjuan commented Oct 22, 2019

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

@mdjermanovic

This comment has been minimized.

Copy link
Member

mdjermanovic commented Oct 23, 2019

@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

This comment has been minimized.

Copy link
Member Author

yeonjuan commented Oct 23, 2019

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

Copy link
Member

mdjermanovic left a comment

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

@platinumazure platinumazure merged commit 7dffe48 into eslint:master Oct 23, 2019
16 checks passed
16 checks passed
Verify Files
Details
Test (ubuntu-latest, 8.x)
Details
Test (ubuntu-latest, 10.x)
Details
Test (ubuntu-latest, 12.x)
Details
Test (windows-latest, 12.x)
Details
Test (macOS-latest, 12.x)
Details
Browser Test
Details
commit-message Commit message follows guidelines
Details
continuous-integration Build #20191023.1 succeeded
Details
continuous-integration (Test on Node.js 10 (Linux)) Test on Node.js 10 (Linux) succeeded
Details
continuous-integration (Test on Node.js 12 (Linux)) Test on Node.js 12 (Linux) succeeded
Details
continuous-integration (Test on Node.js 12 (Windows)) Test on Node.js 12 (Windows) succeeded
Details
continuous-integration (Test on Node.js 12 (macOS)) Test on Node.js 12 (macOS) succeeded
Details
continuous-integration (Test on Node.js 8 (Linux)) Test on Node.js 8 (Linux) succeeded
Details
licence/cla Contributor License Agreement is signed.
Details
release-monitor No patch release is pending
Details
@platinumazure

This comment has been minimized.

Copy link
Member

platinumazure commented Oct 23, 2019

Merged-- thanks @yeonjuan for contributing!

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Linked issues

Successfully merging this pull request may close these issues.

None yet

4 participants
You can’t perform that action at this time.