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

Open
wants to merge 6 commits into
base: master
from

Conversation

Projects
None yet
2 participants
@golopot
Copy link
Contributor

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.

This comment has been minimized.

Copy link
@ljharb

ljharb Apr 18, 2019

Collaborator

this seems like consistent-multiline would be a better name

This comment has been minimized.

Copy link
@golopot

golopot Apr 18, 2019

Author Contributor

I think that consistent multiline sound like a consistent version of multiline, but that is not true. Because multipline is already consistent.

This comment has been minimized.

Copy link
@golopot

golopot Apr 18, 2019

Author Contributor

I think multilinelax is fit because it is like multiline, but more permissive when the expression is single line.

This comment has been minimized.

Copy link
@ljharb

ljharb Apr 18, 2019

Collaborator

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?

This comment has been minimized.

Copy link
@golopot

golopot Apr 18, 2019

Author Contributor

multiline + consistent = multiline. multiline already implied consistent.

This comment has been minimized.

Copy link
@ljharb

ljharb Apr 18, 2019

Collaborator

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

This comment has been minimized.

Copy link
@golopot

golopot Apr 19, 2019

Author Contributor

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).

This comment has been minimized.

Copy link
@golopot

golopot Apr 21, 2019

Author Contributor

How about this option schema?

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

This comment has been minimized.

Copy link
@ljharb

ljharb May 5, 2019

Collaborator

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

This comment has been minimized.

Copy link
@golopot

golopot May 7, 2019

Author Contributor

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?

@golopot golopot force-pushed the golopot:jsx-curly-newline branch from cf76a40 to 410521a May 7, 2019

golopot added some commits May 7, 2019

@ljharb
Copy link
Collaborator

left a comment

I think the overall direction here is solid, thanks.

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 docs/rules/jsx-curly-newline.md Outdated

@ljharb ljharb added the new rule label May 13, 2019

Apply suggestions: change "around" to "inside"
Co-Authored-By: Jordan Harband <ljharb@gmail.com>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
You can’t perform that action at this time.