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 the newline with object literals bug #1180

Merged
merged 1 commit into from
May 9, 2017

Conversation

fatfisz
Copy link
Contributor

@fatfisz fatfisz commented May 7, 2017

From comment #857 (comment):

Found another bug:

<App foo={
{ bar: true, baz: true }
} />;

This would get 2 errors using the default settings: There should be no space after '{' and There should be no space before '}'.

This is because conditions for the object literal spacing were written so that the second one was never reached:

          if (sourceCode.isSpaceBetweenTokens(first, second)) {
            reportNoBeginningSpace(node, first);
          } else if (!config.allowMultiline && isMultiline(first, second)) {
            reportNoBeginningNewline(node, first, config);
          }

If the first condition fails, isMultiline will return false - there is no newline.

Will fix this and add appropriate tests.

code: '<App foo={ {bar:baz} } />;',
options: ['always'],
parserOptions: parserOptions
}, {
code: [
'<App foo={',
'bar',
Copy link
Member

Choose a reason for hiding this comment

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

can we keep this test case as well?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

This test is identical with the one just below it, so I suggest it stays removed.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

(or is there any reason for the duplicated test?)

Copy link
Member

Choose a reason for hiding this comment

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

good call, i didn't realize it was duplicated

}, {
message: 'There should be no newline before \'}\''
}],
parserOptions: parserOptions
Copy link
Member

Choose a reason for hiding this comment

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

Now that we support node 4+, all these can just be parserOptions (only the new examples; we'll change all the rest in their own commit)

Copy link
Contributor Author

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 having only a part the code conform to the new convention is a good idea, especially since this convention can be applied automatically. How about I make a separate PR which replaces all occurences of parserOptions: parserOptions with parserOptions everywhere?

Copy link
Member

Choose a reason for hiding this comment

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

Sure, that's fine, I suppose.

@fatfisz fatfisz force-pushed the fix-newline-with-object-literals branch from be836c6 to 1534bdb Compare May 8, 2017 20:41
@fatfisz
Copy link
Contributor Author

fatfisz commented May 8, 2017

I've rebased this. There's only the unresolved problem of the duplicate test left.

@ljharb ljharb requested a review from yannickcr May 9, 2017 01:48
@ljharb ljharb added the bug label May 9, 2017
@ljharb ljharb merged commit f111e45 into jsx-eslint:master May 9, 2017
@fatfisz fatfisz deleted the fix-newline-with-object-literals branch May 9, 2017 19:41
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Development

Successfully merging this pull request may close these issues.

None yet

3 participants