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

Update: Add "multiline" type to padding-line-between-statements #8668

Conversation

@Lobstrosity
Copy link
Contributor

Lobstrosity commented May 30, 2017

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

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

What changes did you make? (Give an overview)

Adds multiline-expression statement type to existing padding-line-between-statements rule.

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

n/a

What rule do you want to change?

padding-line-between-statements

Does this change cause the rule to produce more or fewer warnings?

More (or at least never fewer)

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

Adds multiline-expression property to existing StatementTypes object (basically a copy/paste of the existing expression property and a copy/paste of the line numbers check from existing multiline-block-like property).

Please provide some example code that this change will affect:

Given the following rule:

"padding-line-between-statements": [
  "error",
  { "blankLine": "always", "prev": "*", "next": "multiline-expression" }
]

The following code (single-line expression without padding line) is correct:

const promise = fetch();
promise.then(console.dir);

The following code (multi-line expression with padding line) is correct:

const promise = fetch();

promise.then(data => {
  console.dir(data);
});

The following code (multi-line expression without padding line) is incorrect:

const promise = fetch();
promise.then(data => {
  console.dir(data);
});

What does the rule currently do for this code?

Currently, no warnings/errors emitted for any of the samples above.

What will the rule do after it's changed?

No warnings/errors will be produced for the correct samples. An error will be produced for the incorrect sample.

@eslintbot

This comment has been minimized.

Copy link

eslintbot commented May 30, 2017

LGTM

@platinumazure

This comment has been minimized.

Copy link
Member

platinumazure commented May 30, 2017

Seems reasonable (and useful), 👍 from me.

@kaicataldo

This comment has been minimized.

Copy link
Member

kaicataldo commented Jun 21, 2017

@eslint/eslint-team Looks like this just needs a champion to move forward.

@not-an-aardvark

This comment has been minimized.

Copy link
Member

not-an-aardvark commented Sep 16, 2017

Thanks for your interest in improving ESLint. Unfortunately, it looks like this proposal didn't get consensus from the team, so I'm closing it. We define consensus as having three 👍s from team members, as well as a team member willing to champion the proposal. This is a high bar by design -- we can't realistically accept and maintain every feature request in the long term, so we only accept feature requests which are useful enough that there is consensus among the team that they're worth adding.

Since ESLint is pluggable and can load custom rules at runtime, the lack of consensus among the ESLint team doesn't need to be a blocker for you using this in your project, if you'd find it useful. It just means that you would need to implement this as a custom rule yourself, rather than using a bundled rule that is packaged with ESLint.

@aladdin-add

This comment has been minimized.

Copy link
Member

aladdin-add commented Jan 25, 2018

since #9491 has been accept, shall we reopen this PR, and marked accepted?

@platinumazure platinumazure reopened this Jan 25, 2018
@jsf-clabot

This comment has been minimized.

Copy link

jsf-clabot commented Jan 25, 2018

CLA assistant check
All committers have signed the CLA.

@platinumazure platinumazure added accepted and removed evaluating labels Jan 25, 2018
@platinumazure

This comment has been minimized.

Copy link
Member

platinumazure commented Jan 25, 2018

I've reopened this PR, thanks!

@Lobstrosity If you're still willing to work on this, here's what we need:

  • We'll the CLA signed before we can accept this contribution.
  • In addition, please make sure that this PR addresses the proposal we accepted in #9491, or at least has only minor differences. (I haven't checked yet, I'm not saying this PR definitely doesn't match the proposal.)

Thanks for your patience as we worked through our process!

Lobstrosity added 2 commits Jan 28, 2018
…expression-statement-type
@Lobstrosity

This comment has been minimized.

Copy link
Contributor Author

Lobstrosity commented Jan 28, 2018

@platinumazure: Thank you for re-evaluating.

I have signed the CLA.

And I have added test cases to confirm that with this rule:

{ blankLine: "always", prev: "multiline-expression", next: "return" }

The following are correct:

() => {
  someArray.forEach(x => doSomething(x));
  return theThing;
}
() => {
  someArray.forEach(
    x => doSomething(x)
  );

  return theThing;
}

But this is incorrect:

() => {
  someArray.forEach(
    x => doSomething(x)
  );
  return theThing;
}
Copy link
Member

platinumazure left a comment

LGTM, thanks! (It'd be nice to make the test cases span multiple lines using template strings, but that could be done in a future PR.)

@platinumazure

This comment has been minimized.

Copy link
Member

platinumazure commented Feb 2, 2018

@Lobstrosity Thanks, awesome work! I apologize for letting this sit so long. Not sure we can get this into today's release (at least, I'd like one more reviewer before merging); but if we miss today's, it should definitely get in next release. Thanks for contributing!

@platinumazure platinumazure changed the title Update: Add multiline expression statement type Update: Add "multiline" statement type to padding-line-between-statements Feb 3, 2018
@platinumazure platinumazure changed the title Update: Add "multiline" statement type to padding-line-between-statements Update: Add "multiline" type to padding-line-between-statements Feb 3, 2018
@platinumazure

This comment has been minimized.

Copy link
Member

platinumazure commented Feb 3, 2018

Thanks for contributing, @Lobstrosity! And I'm glad we could finally get this in-- thanks for your patience!

@platinumazure platinumazure merged commit 1da1ada into eslint:master Feb 3, 2018
5 checks passed
5 checks passed
commit-message PR title follows commit message guidelines
Details
continuous-integration/appveyor/pr AppVeyor build succeeded
Details
continuous-integration/travis-ci/pr The Travis CI build passed
Details
licence/cla Contributor License Agreement is signed.
Details
release-monitor No patch release is pending
Details
This was referenced Mar 19, 2018
@renovate renovate bot mentioned this pull request Mar 23, 2018
@eslint eslint bot locked and limited conversation to collaborators Aug 3, 2018
@eslint eslint bot added the archived due to age label Aug 3, 2018
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Projects
None yet
Linked issues

Successfully merging this pull request may close these issues.

None yet

7 participants
You can’t perform that action at this time.