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: function-paren-newline rule (fixes #6074) #8102
Conversation
@not-an-aardvark, thanks for your PR! By analyzing the history of the files in this pull request, we identified @mysticatea, @kaicataldo and @gyandeeps to be potential reviewers. |
98800e5
to
d550869
Compare
LGTM |
Do we need an autofix in the first release of this rule? We could put a fixer in for 4.0 but release a usable rule in a minor release, no? |
Looks like we have some merge conflicts here. No need to rush this though since it doesn't have to be released in a major version. |
docs/rules/function-paren-newline.md
Outdated
@@ -0,0 +1,225 @@ | |||
# enforce consistent line breaks inside function parentheses (function-paren-newline) | |||
|
|||
(fixable) The `--fix` option on the [command line](../user-guide/command-line-interface#fix) automatically fixes problems reported by this rule. |
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.
We don't need this anymore.
Does anyone have any thoughts on how to address the usability issues described in the comment at the top? |
I think the only way to fix this usability issue is to introduce yet another option. While option 2 is reasonable, I personally prefer option 1. I also see an issue of requiring closing brace to be on it's own line (I personally prefer it to be on the same line as last argument). |
What is the status on this? |
I've been thinking about this more. What if we added a selector option to the rule to allow greater configurability? Would that solve all the use cases we need? For example, maybe people could use that to enforce parens for nested functions. |
Is there an update on this issue? |
Could we maybe add a
I think that would be a sensible default, as well. Also, does this apply to function calls, function declarations, or both? If both, is it worth creating rules for (or supporting options in this rule for) different behaviors for function calls vs function declarations? |
I agree, adding a I think we should make
It applies to all "function parens" (i.e. the parentheses around the arguments in CallExpressions and NewExpressions, and the parentheses around the parameters in FunctionDeclarations, FunctionExpressions, and ArrowFunctionExpressions). |
👍, I had that in mind as well.
I'm okay with this for now, but does it not seem likely that we'll get a request to support different behavior? (But that can certainly be added later as a non-breaking change.) |
d550869
to
7c28d95
Compare
LGTM |
LGTM |
Dismissing this review because a lot of time has passed and the proposal has changed a bit.
07265ca
to
a0bd3fc
Compare
LGTM |
@eslint/eslint-team This should be ready to merge after review. Sorry about the long bikeshedding delay. |
a0bd3fc
to
5fb4745
Compare
LGTM |
5fb4745
to
bc41861
Compare
LGTM |
When do you guys plan to release a new eslint version with this feature? |
|
Is there a proposed solution for @jainanshul's scenario above? |
No. Currently, the rule is working as designed, but if you'd like to propose an enhancement to this rule, please create a new issue. |
What is the purpose of this pull request? (put an "X" next to item)
[x] New rule (see #6074)
What changes did you make? (Give an overview)
This implements the
function-paren-newline
rule as described in #6074, and enables it on the ESLint codebase for dogfooding. The rule applies to all function parameters, as well asCallExpression
/NewExpression
arguments.I decided to use
multiline
as the default, instead ofalways
as described in #6074, becausealways
seems like a very unusual style.Is there anything you'd like reviewers to focus on?
I'm unsure of the correct behavior for the
multiline
option. There are two possible behaviors that I can think of, which are subtly different:If option 1 is used, the following code is reported, because the arguments span multiple lines. I think this is very undesirable.
If option 2 is used, the rule reports code like this, which is slightly undesirable:
Currently, option 2 is implemented, but I'm concerned that it makes code less readable in some cases.
The rule can produce some very long lines. For example, the following code gets reported:
I'm concerned that this can make the code less readable.
Due to Modify rules for handling indentation of closing parentheses #7522, the autofixer for this rule can be difficult to use. For example:
Since the
indent
rule doesn't report or fix the indentation of the closing)
, it usually ends up in the wrong place. The indent rewrite fixes this issue, so maybe we should wait until 4.0.0 before releasing this rule.