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

Fix: ignore aligning single line in key-spacing (fixes #11414) #12652

Merged
merged 3 commits into from Dec 20, 2019
Merged

Fix: ignore aligning single line in key-spacing (fixes #11414) #12652

merged 3 commits into from Dec 20, 2019

Conversation

@yeonjuan
Copy link
Member

@yeonjuan yeonjuan commented Dec 7, 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)
fix #11414
make it ignore aligning single line properties.

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

Copy link
Member

@kaicataldo kaicataldo left a comment

Thanks for working on this!

lib/rules/key-spacing.js Show resolved Hide resolved
tests/lib/rules/key-spacing.js Show resolved Hide resolved
yeonjuan added 2 commits Dec 10, 2019
@yeonjuan yeonjuan requested a review from kaicataldo Dec 14, 2019
Copy link
Member

@kaicataldo kaicataldo left a comment

I'm not sure this fixes the problem in full (see comments). That being said, I recognize that the behavior I'm suggesting below is a lot more work. I would be fine with landing this as-is because it's passing through the options and does seem like an improvement 👍

output: [
"var obj = {",
" foo : 1,",
" 'bar' : 2, baz : 3, longlonglong: 4",

This comment has been minimized.

@kaicataldo

kaicataldo Dec 14, 2019
Member

I understand why this is the way it is, but I wonder if this actually makes sense?

Feels like it should be:

var obj = {
    foo  : 1,
    'bar': 2, baz: 3, longlonglong: 4
};

or

var obj = {
    foo: 1, 'bar'       : 2,
    baz: 3, longlonglong: 4
};
output: [
"var obj = {",
" foo : 1,",
" 'bar' : 2, baz : 3,",

This comment has been minimized.

@kaicataldo

kaicataldo Dec 14, 2019
Member

Same comment as above.

This comment has been minimized.

@yeonjuan

yeonjuan Dec 14, 2019
Author Member

@kaicataldo
Yes, I agree :) Can I work on that in different PR?.

Copy link
Member

@btmills btmills left a comment

Also fine landing as-is. Thanks for delving into this rule, @yeonjuan!

@btmills btmills merged commit c5c7086 into eslint:master Dec 20, 2019
17 of 18 checks passed
17 of 18 checks passed
@github-actions
Verify Files
Details
@github-actions
Test (ubuntu-latest, 13.x)
Details
@github-actions
Test (ubuntu-latest, 12.x)
Details
@github-actions
Test (ubuntu-latest, 10.x)
Details
@github-actions
Test (ubuntu-latest, 8.x)
Details
@github-actions
Test (ubuntu-latest, 8.10.0)
Details
@github-actions
Test (windows-latest, 12.x)
Details
@github-actions
Test (macOS-latest, 12.x)
Details
@github-actions
Browser Test Browser Test
Details
@eslint-deprecated
commit-message PR title follows commit message guidelines
Details
@azure-pipelines
continuous-integration Build #20191212.1 succeeded
Details
@azure-pipelines
continuous-integration (Test on Node.js 10 (Linux)) Test on Node.js 10 (Linux) succeeded
Details
@azure-pipelines
continuous-integration (Test on Node.js 12 (Linux)) Test on Node.js 12 (Linux) succeeded
Details
@azure-pipelines
continuous-integration (Test on Node.js 12 (Windows)) Test on Node.js 12 (Windows) succeeded
Details
@azure-pipelines
continuous-integration (Test on Node.js 12 (macOS)) Test on Node.js 12 (macOS) succeeded
Details
@azure-pipelines
continuous-integration (Test on Node.js 8 (Linux)) Test on Node.js 8 (Linux) succeeded
Details
licence/cla Contributor License Agreement is signed.
Details
@eslint-deprecated
release-monitor No patch release is pending
Details
@yeonjuan yeonjuan deleted the yeonjuan:fix-only-single-line branch Jan 3, 2020
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Projects
None yet
Linked issues

Successfully merging this pull request may close these issues.

3 participants