Skip to content

Added allowMultiline property to the jsx-curly-spacing rule #156

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

Merged
merged 1 commit into from
Jul 20, 2015

Conversation

mathieumg
Copy link
Contributor

What do you think about this? I had hoped it would be like that by default when I first read about that rule being introduced! Which prompts my next question: should allowMultiline be on by default? I currently put it off by default and I'm ignoring it when the option is set to "always" as to not break current users (new warnings/errors) that do:

<Hello name={
  firstname
} />;

using

"jsx-curly-spacing": [2, "always"]

If it were on by default, I could leverage the option in all cases, meaning users would have a way to enforce:

// Valid
<Hello name={ firstname } />;

// Invalid
<Hello name={
  firstname
} />;

with

"jsx-curly-spacing": [2, "always", {"allowMultiline": false}]

However, current users relying on

<Hello name={
  firstname
} />;

to warn with the current defaults, would not be warned anymore.

@mathieumg mathieumg force-pushed the jsxcurlyspacing_multiline branch 2 times, most recently from 15a6907 to 86be3e0 Compare July 16, 2015 13:48
@yannickcr
Copy link
Member

Thanks for this!

I think it's ok to make a small break on this rule to make it more flexible.

@mathieumg
Copy link
Contributor Author

So, which break is it? 😛 Default to true, but rule also considered with "always" if set to false? (Last example I showed)

If so, I'll update the PR!

@moll
Copy link

moll commented Jul 19, 2015

👍 This would then match http://eslint.org/docs/rules/object-curly-spacing.html which allows multilines in the never setting.

@yannickcr
Copy link
Member

So, which break is it? 😛 Default to true, but rule also considered with "always" if set to false? (Last example I showed)

Yep, that break ;)

@mathieumg mathieumg force-pushed the jsxcurlyspacing_multiline branch from 86be3e0 to b779adf Compare July 20, 2015 15:24
@@ -79,16 +112,28 @@ module.exports = function(context) {
* @returns {void}
*/
function validateBraceSpacing(node, first, second, penultimate, last) {
Copy link
Contributor Author

Choose a reason for hiding this comment

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

Refactored this part a bit because it was getting hard to follow! I don't know what your stance is on early returns to avoid deep nesting, but I can change this to an else statement if that is what you prefer.

Copy link
Member

Choose a reason for hiding this comment

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

LGTM

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

Successfully merging this pull request may close these issues.

None yet

3 participants