Skip to content

Update: support single argument on newline with function-paren-newline#11406

Merged
not-an-aardvark merged 2 commits intoeslint:masterfrom
gwer:function-paren-newline-consistent-arguments
Apr 2, 2019
Merged

Update: support single argument on newline with function-paren-newline#11406
not-an-aardvark merged 2 commits intoeslint:masterfrom
gwer:function-paren-newline-consistent-arguments

Conversation

@gwer
Copy link
Contributor

@gwer gwer commented Feb 16, 2019

What is the purpose of this pull request? (put an "X" next to item)

  • Documentation update
  • Bug fix (template)
  • New rule (template)
  • Changes an existing rule (template)
  • Add autofixing to a rule
  • Add a CLI option
  • Add something to the core
  • Other, please explain:

What rule do you want to change?
function-paren-newline

How will the change be implemented? (New option, new default behavior, etc.)?
New option consistent-arguments.

What changes did you make? (Give an overview)

"consistent-arguments" requires consistent usage of linebreaks for each pair of parentheses and parameters/arguments. It allows linebreaks inside function parentheses if there is only one parameter/argument.

Examples of incorrect code for this rule with the "consistent-arguments" option:

/* eslint function-paren-newline: ["error", "consistent-arguments"] */

function foo(bar,
  baz
) {}

var foo = function(bar,
  baz
) {};

var foo = (
  bar,
  baz) => {};

foo(
  bar,
  baz);

foo(
  bar, qux,
  baz
);

Examples of correct code for this rule with the consistent "consistent-arguments" option:

/* eslint function-paren-newline: ["error", "consistent-arguments"] */

function foo(
  bar,
  baz
) {}

var foo = function(bar, baz) {};

var foo = (
  bar
) => {};

foo(
  function() {
    return baz;
  }
);

Is there anything you'd like reviewers to focus on?

There are some issues about this problem :: #9286 :: #9411

For multiline option this is obvious behavior when you can use single argument on their own line. But now for this you should use less strict consistent option.

console.log(foo,
  bar,
  baz);

Airbnb JavaScript Style Guide define this code as bad. But their eslint config now use consistent option that miss many bad cases because multiline option disallow to use single argument on their own line. Until then they were forced to use additional parens as workaround.

@eslint-deprecated eslint-deprecated bot added the triage An ESLint team member will look at this issue soon label Feb 16, 2019
@g-plane g-plane added enhancement This change enhances an existing feature of ESLint evaluating The team will evaluate this issue to decide whether it meets the criteria for inclusion rule Relates to ESLint's core rules and removed triage An ESLint team member will look at this issue soon labels Feb 16, 2019
@gwer gwer force-pushed the function-paren-newline-consistent-arguments branch from 3069014 to df8699c Compare February 21, 2019 12:33
@gwer
Copy link
Contributor Author

gwer commented Feb 24, 2019

🤔

@ilyavolodin
Copy link
Member

Looks like this PR has a lot of community support, but no support from the team. Anyone on ESLint team wants to support this change?

@gwer
Copy link
Contributor Author

gwer commented Mar 6, 2019

@g-plane @platinumazure, what do you think about this PR?

@g-plane
Copy link
Member

g-plane commented Mar 6, 2019

IMHO, we can accept it if the community support it.

@platinumazure
Copy link
Member

@g-plane Would you like to champion this issue?

@g-plane
Copy link
Member

g-plane commented Mar 6, 2019

@platinumazure I will and I may review this PR tomorrow.

@g-plane g-plane self-assigned this Mar 6, 2019
@g-plane g-plane added accepted There is consensus among the team that this change meets the criteria for inclusion and removed evaluating The team will evaluate this issue to decide whether it meets the criteria for inclusion labels Mar 6, 2019
@aladdin-add
Copy link
Member

aladdin-add commented Mar 6, 2019

@g-plane to accept a rule change, we need a champion and 3+ 👍from the team. That is to say, the issue is not "accepted" yet.

see https://eslint.org/docs/maintainer-guide/issues#accepting-issues

@platinumazure platinumazure added evaluating The team will evaluate this issue to decide whether it meets the criteria for inclusion and removed accepted There is consensus among the team that this change meets the criteria for inclusion labels Mar 6, 2019
@g-plane
Copy link
Member

g-plane commented Mar 7, 2019

Ok, I'm sorry.

@gwer
Copy link
Contributor Author

gwer commented Mar 19, 2019

@g-plane, how is the championing progressing?

@platinumazure
Copy link
Member

@gwer I've left a 👍 on the issue so we're at least 1/3 of the way to team consensus. Unfortunately, I can't tell which other team members might have left 👍 on the issue because we have a large number of community members who have left 👍 and GitHub only shows a subset (it doesn't even show my name).

@g-plane It might be worth pinging team members on the team Gitter chat to try to get this accepted.

@g-plane
Copy link
Member

g-plane commented Mar 19, 2019

I've left a 👍 before.

@platinumazure
Copy link
Member

@g-plane Awesome. That said-- you're champion, so we need 3 other team members besides you to leave 👍. As I noted, I've already done so.

@gwer
Copy link
Contributor Author

gwer commented Mar 28, 2019

Hmm, I think today is a good day to ping team members (=

@ilyavolodin ilyavolodin added accepted There is consensus among the team that this change meets the criteria for inclusion and removed evaluating The team will evaluate this issue to decide whether it meets the criteria for inclusion labels Mar 29, 2019
@ilyavolodin
Copy link
Member

This is now accepted, since we have more then enough support from the team for it.

Copy link
Member

@g-plane g-plane left a comment

Choose a reason for hiding this comment

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

LGTM, thanks!

Copy link
Member

@not-an-aardvark not-an-aardvark left a comment

Choose a reason for hiding this comment

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

Thanks for the PR! I left a comment -- I'm having trouble understanding what the intended behavior is based on the documentation.

@gwer
Copy link
Contributor Author

gwer commented Apr 2, 2019

I hope one day we will merge this PR :D

Copy link
Member

@not-an-aardvark not-an-aardvark left a comment

Choose a reason for hiding this comment

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

LGTM, thanks!

@not-an-aardvark not-an-aardvark merged commit 2f8ae13 into eslint:master Apr 2, 2019
@eslint-deprecated eslint-deprecated bot locked and limited conversation to collaborators Sep 30, 2019
@eslint-deprecated eslint-deprecated bot added the archived due to age This issue has been archived; please open a new issue for any further discussion label Sep 30, 2019
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.

Labels

accepted There is consensus among the team that this change meets the criteria for inclusion archived due to age This issue has been archived; please open a new issue for any further discussion enhancement This change enhances an existing feature of ESLint rule Relates to ESLint's core rules

Projects

None yet

Development

Successfully merging this pull request may close these issues.

8 participants

Comments