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

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-deprecated eslint-deprecated bot added the triage An ESLint team member will look at this issue soon label Sep 4, 2019
@mdjermanovic
Copy link
Member Author

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 There is consensus among the team that this change meets the criteria for inclusion bug ESLint is working incorrectly rule Relates to ESLint's core rules and removed triage An ESLint team member will look at this issue soon labels Sep 4, 2019
Copy link
Member

@g-plane g-plane left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM, thanks!

@platinumazure
Copy link
Member

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

@mdjermanovic
Copy link
Member Author

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") {
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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?

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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
@ilyavolodin ilyavolodin deleted the issue11902 branch September 14, 2019 00:46
@eslint-deprecated eslint-deprecated bot locked and limited conversation to collaborators Mar 13, 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 Mar 13, 2020
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
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 rule Relates to ESLint's core rules
Projects
None yet
Development

Successfully merging this pull request may close these issues.

object-curly-spacing with line comment
5 participants