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

New: jsx-curly-newline rule #2240

Merged
merged 1 commit into from Jun 5, 2019
Merged

Conversation

golopot
Copy link
Contributor

@golopot golopot commented Apr 14, 2019

fixes #1493

This rule is based on eslint core rule function-paren-newline. Its options is similar to that of function-paren-newline, array-bracket-newline, and object-curly-newline. However there is one new option multiline-lax (default), which is the same as multiline but tolerate cases like

<div>
{
  foo
}
</div>

, that is, single line expressions are allowed to have both newlines.

Separately, since this commit is modified copy of the eslint core rule function-paren-newline.js, I need help on how to deal with the license things.


* `consistent` enforces either both of linebreaks around curlies are present, or none are present.
* `multiline` enforces that when the contained expression spans multiple line, require both linebreaks. When the contained expression is single line, disallow both line breaks.
* `multiline-lax` (default) enforces that when the contained expression spans multiple line, require both linebreaks. When the contained expression is single line, disallow inconsistent line break.
Copy link
Member

Choose a reason for hiding this comment

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

this seems like consistent-multiline would be a better name

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 think that consistent multiline sound like a consistent version of multiline, but that is not true. Because multipline is already consistent.

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 think multilinelax is fit because it is like multiline, but more permissive when the expression is single line.

Copy link
Member

Choose a reason for hiding this comment

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

Looking at eslint core, there's a number of rules that have an object option with a multiline boolean, and a consistent boolean - if we only allowed multiline, multiline + consistent, and consistent, that seems like it'd cover it more clearly?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

multiline + consistent = multiline. multiline already implied consistent.

Copy link
Member

Choose a reason for hiding this comment

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

Which don't make sense? We can use schema validation to only permit the desired combinations.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Now I feel I was wrong thinking most of them don't make sense. However I think it is safe to not support the ignore option (just use consistent).

Copy link
Contributor Author

Choose a reason for hiding this comment

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

How about this option schema?

"never" |
"consistent" |
{
  "single-line": "require" | "forbid" | "consistent" ,
  "multiline": "require" | "forbid" | "consistent" ,
}

Copy link
Member

Choose a reason for hiding this comment

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

That seems good; let's update the PR and see what the test cases and docs for each combination look like :-D

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Now I feel I am ok to live with the simple option schema "never" | "consistent", dropping the multiline option. Are you interested in this direction?

Copy link
Member

@ljharb ljharb left a comment

Choose a reason for hiding this comment

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

I think the overall direction here is solid, thanks.

docs/rules/jsx-curly-newline.md Outdated Show resolved Hide resolved
docs/rules/jsx-curly-newline.md Outdated Show resolved Hide resolved
docs/rules/jsx-curly-newline.md Outdated Show resolved Hide resolved
docs/rules/jsx-curly-newline.md Outdated Show resolved Hide resolved
@ljharb ljharb merged commit 44e568a into jsx-eslint:master Jun 5, 2019
@golopot
Copy link
Contributor Author

golopot commented Jun 6, 2019

I forgot to add this rule to the README.

@ljharb
Copy link
Member

ljharb commented Jun 6, 2019

@golopot a followup PR would be great

@golopot golopot deleted the jsx-curly-newline branch June 6, 2019 09:09
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.

jsx-curly-spacing should support "consistent" option
2 participants