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

Multiline expressions in padding-line-between-statements rule #9491

Closed
SeanEire opened this issue Oct 20, 2017 · 10 comments
Closed

Multiline expressions in padding-line-between-statements rule #9491

SeanEire opened this issue Oct 20, 2017 · 10 comments
Assignees
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

Comments

@SeanEire
Copy link

SeanEire commented Oct 20, 2017

Tell us about your environment

  • ESLint Version: 4.9.0
  • Node Version: 6.11.3
  • npm Version: 3.10.10

What parser (default, Babel-ESLint, etc.) are you using?
AirBnb
Please show your full configuration:

Configuration
{
    "parserOptions": {
        "ecmaVersion": 6,
        "sourceType": "script"
    },
    "env": {
        "node": true
    },
    "extends": "airbnb-base",
    "rules": {
        "arrow-parens": ["error", "always"],
        "comma-dangle": ["error", "never"],
        "consistent-return": "off",
        "indent": ["error", 2],
        "max-len": ["error", 200],
        "no-multiple-empty-lines": ["error", {"max": 1, "maxEOF": 1}],
        "no-plusplus": "off",
        "no-underscore-dangle": ["off"],
        "no-use-before-define": ["error", { "functions": false }],
        "object-shorthand": ["off"],
        "padding-line-between-statements": ["error", 
            {"blankLine": "always", "prev": "block", "next": "*"},
            {"blankLine": "always", "prev": "function", "next": "*"},
            {"blankLine": "always", "prev": "block", "next":"*"},
            {"blankLine": "always", "prev": ["cjs-export"], "next": "*"},
            {"blankLine": "always", "prev": ["*"], "next": "cjs-export"}
        ],
        "strict": ["error", "global"]
    }
}

What did you do? Please include the actual source code causing the issue.
Would it be or is it already possible to distinguish multiline from single line expressions in the padding-line-between-statements rule? For example:

Distinguish this:

someArray.forEach((x) => doSomething(x));

From this:

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

What did you expect to happen?
I would like to be able to identify multiline expressions in order to enforce blank lines between them and certain other statements.

What actually happened? Please include the actual, raw output from ESLint.
No ability to distinguish multi line expressions from single line expressions.

@eslintbot eslintbot added the triage An ESLint team member will look at this issue soon label Oct 20, 2017
@VictorHom
Copy link
Member

VictorHom commented Oct 21, 2017

Hey!

Just want to make sure I understand.

For the case of someArray.forEach((x) => doSomething(x));, should anything happen?

and for

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

would you expect the correction output to be:

someArray.forEach((x) => {
  doSomething(x);

  doAnotherThing(x);
});

You might have to write a custom rule to enforce this style. I couldn't find a particular rule that would do the exact thing you're looking for.

@eslintbot
Copy link

Hi @SeanEire, thanks for the issue. It looks like there's not enough information for us to know how to help you.

If you're reporting a bug, please be sure to include:

  1. The version of ESLint you are using (run eslint -v)
  2. What you did (the source code and ESLint configuration)
  3. The actual ESLint output complete with numbers
  4. What you expected to happen instead

Requesting a new rule? Please see Proposing a New Rule for instructions.

Requesting a rule change? Please see Proposing a Rule Change for instructions.

If it's something else, please just provide as much additional information as possible. Thanks!

@VictorHom VictorHom added needs info Not enough information has been provided to triage this issue question This issue asks a question about ESLint labels Oct 21, 2017
@SeanEire
Copy link
Author

SeanEire commented Oct 23, 2017

Thanks for getting back to me @VictorHom ! I'm actually looking to enforce a line break after a multi-line expression preceding a return statement.

Now I can use this rule for single/multi line expressions {"blankLine": "always", "prev": "expression", "next": "return"}

This enforces:

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

return theThing;

But also for single line expressions:

doSomething();

return theThing;

The goal is something like: {"blankLine": "always", "prev": "multiline-expression", "next": "return"}

Which would only enforce line break after multiline expressions:

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

return theThing;

And not single line expressions:

doSomething();
return theThing;

@VictorHom
Copy link
Member

VictorHom commented Oct 26, 2017

@SeanEire I see. For the multiline blocks, are your typical cases with constructs like a ForEach?

I'm look through the documentation for https://eslint.org/docs/rules/padding-line-between-statements and there is an option for {"blankLine": "never", "prev": "for", "next": "return"}], which doesn't work for a forEach statement. It states it only works for This matches all statements that the first token is for keyword.

Wondering if
(a) we can/should support forEach statements in a way that would solve the case you posted.

My concern is around cases single line expressions that are on multiple lines in the multiline expression example that was posted earlier:

foo(
  param1,
  param2,
  param3
)

@VictorHom VictorHom removed the triage An ESLint team member will look at this issue soon label Oct 26, 2017
@SeanEire
Copy link
Author

@VictorHom ForEach would be one example yes. My most typical case however is with the usage of promise chains. What I'd really like to do is enforce line breaks before/after the chain statement.

Something like this:

const deferred = q.defer();

doSomething()
  .then(doAnotherThing)
  .then(deferred.resolve)
  .fail(deferred.reject);

return deferred.promise;

@not-an-aardvark not-an-aardvark 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 needs info Not enough information has been provided to triage this issue question This issue asks a question about ESLint labels Oct 29, 2017
@not-an-aardvark
Copy link
Member

It looks like this was also proposed in #8668 but no one on the team ended up championing the proposal. We can see if anyone is interested in supporting it this time around.

@SeanEire
Copy link
Author

@not-an-aardvark any chance of this being added in one form or another?

@not-an-aardvark
Copy link
Member

@eslint/eslint-team It looks like this issue just needs a champion to move forward.

@aladdin-add
Copy link
Member

great! I'll champion this, marked as accepted.

@aladdin-add aladdin-add 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 Dec 30, 2017
@aladdin-add aladdin-add self-assigned this Dec 30, 2017
@aladdin-add
Copy link
Member

closing, as #8668 get merged.

@eslint-deprecated eslint-deprecated bot locked and limited conversation to collaborators Aug 3, 2018
@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 Aug 3, 2018
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

No branches or pull requests

5 participants