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: Allow line comment exception in object-curly-spacing (fixes #11902) #12216

Merged
merged 1 commit into from Sep 14, 2019

Conversation

@mdjermanovic
Copy link
Member

mdjermanovic commented Sep 4, 2019

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

[X] Bug fix #11902
[X] Changes an existing rule

This PR allows one particular exception in object-curly-spacing default behavior, and nothing more:

  • When the option is never, spaces will be allowed between { and a line comment //.

All other combinations of options and comments are unchanged.

What rule do you want to change?

object-curly-spacing

Does this change cause the rule to produce more or fewer warnings?

fewer

How will the change be implemented? (New option, new default behavior, etc.)?

new default behavior

Please provide some example code that this change will affect:

/*eslint object-curly-spacing: ["error", "never"]*/

var foo = { // line comment
  bar: baz
}

var { // line comment
  foo 
} = bar;

import { // line comment
  bar
} from 'foo'

export { // line comment
  bar
}

export { // line comment
  foo
} from 'bar'

What does the rule currently do for this code?

5 errors.

What will the rule do after it's changed?

No errors.

What changes did you make? (Give an overview)

Don't report disallowed spaces between { and //... tokens when the option is never.

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

There are no details in the documentation on how the rule works with comments inside {}, so this technically isn't a bug.

However, it seems that all other spacing rules that account for comments do allow this particular exception. Namely, space-in-parens, block-spacing and comma-spacing.

@eslint eslint bot added the triage label Sep 4, 2019
@mdjermanovic

This comment has been minimized.

Copy link
Member Author

mdjermanovic commented Sep 4, 2019

Marking as accepted because the issue is accepted as a bug (and it's reasonable given how other similar rules work), but have to note that this changes an undocumented part of the default behavior.

@mdjermanovic mdjermanovic added accepted bug rule and removed triage labels Sep 4, 2019
@g-plane
g-plane approved these changes Sep 6, 2019
Copy link
Member

g-plane left a comment

LGTM, thanks!

@platinumazure

This comment has been minimized.

Copy link
Member

platinumazure commented Sep 7, 2019

Should we consider doing the same thing for block comments? (Not as part of this PR, just asking in general.)

@mdjermanovic

This comment has been minimized.

Copy link
Member Author

mdjermanovic commented Sep 7, 2019

Should we consider doing the same thing for block comments? (Not as part of this PR, just asking in general.)

I think that it makes sense, though it wouldn't be as simple logic as it is for the line comments, I'll open an issue for this.

@@ -167,7 +167,7 @@ module.exports = {
if (options.spaced && !firstSpaced) {
reportRequiredBeginningSpace(node, first);
}
if (!options.spaced && firstSpaced) {
if (!options.spaced && firstSpaced && second.type !== "Line") {

This comment has been minimized.

Copy link
@mysticatea

mysticatea Sep 13, 2019

Member

I'm not sure if this condition is proper. My preference is "if the brace and the next property placed on different lines then the rule does nothing."

({ one, two }) // enforce between `{` and `one`
({ /**/ one, two }) // enforce between `{` and `/**/`
({
    one, two }) // ignore between `{` and `one`.
({ /**/
    one, two }) // ignore between `{` and `/**/`.
({ //
    one, two }) // ignore between `{` and `//`.

I'm not sure how it should handle the space between /**/ and one...

In fact, it may be better if we should separate about the spaces around comments to another rule like space-around-comment and other spacing rules ignore the spaces around comments.

This comment has been minimized.

Copy link
@mdjermanovic

mdjermanovic Sep 13, 2019

Author Member

I think it makes sense to reconsider the logic regarding the comments in this and other rules, and other members seem to agree as well.

It isn't a trivial logic though, maybe to merge this PR at the moment because it fixes the particular problem in #11902, and also matches exactly how the other rules work at the moment?

This comment has been minimized.

Copy link
@mdjermanovic

mdjermanovic Sep 13, 2019

Author Member

Perhaps #12198 could be a good place to implement and test the new logic, that rule at the moment removes comments and also has a bug with parens, so it has to be fixed anyway.

This comment has been minimized.

Copy link
@mysticatea

mysticatea Sep 13, 2019

Member

I'm fine if we merge this PR for now and separate the PR that improves about comment-around spaces.

@ilyavolodin ilyavolodin merged commit f826eab into master Sep 14, 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 #20190904.5 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
@ilyavolodin ilyavolodin deleted the issue11902 branch Sep 14, 2019
@eslint eslint bot locked and limited conversation to collaborators Mar 13, 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.

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