-
-
Notifications
You must be signed in to change notification settings - Fork 2.8k
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
Conversation
docs/rules/jsx-curly-newline.md
Outdated
|
||
* `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. |
There was a problem hiding this comment.
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
There was a problem hiding this comment.
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.
There was a problem hiding this comment.
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.
There was a problem hiding this comment.
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?
There was a problem hiding this comment.
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.
There was a problem hiding this comment.
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.
There was a problem hiding this comment.
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
).
There was a problem hiding this comment.
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" ,
}
There was a problem hiding this comment.
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
There was a problem hiding this comment.
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?
There was a problem hiding this 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.
I forgot to add this rule to the README. |
@golopot a followup PR would be great |
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
, andobject-curly-newline
. However there is one new optionmultiline-lax
(default), which is the same asmultiline
but tolerate cases like, 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.