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

padded-blocks rule - add a multi-line option to this rule #7775

Closed
sscaff1 opened this issue Dec 15, 2016 · 12 comments
Closed

padded-blocks rule - add a multi-line option to this rule #7775

sscaff1 opened this issue Dec 15, 2016 · 12 comments
Assignees
Labels
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 evaluating The team will evaluate this issue to decide whether it meets the criteria for inclusion rule Relates to ESLint's core rules

Comments

@sscaff1
Copy link

sscaff1 commented Dec 15, 2016

What rule do you want to change?
padded-blocks

Does this change cause the rule to produce more or fewer warnings?
Potentially fewer warning depending on the situation.

How will the change be implemented? (New option, new default behavior, etc.)?
Added an option to padded-blocks to specify a minimum number of lines required before the rule takes affect.

Please provide some example code that this change will affect:

Currently in JSCS, we have the rule of requirePaddingNewlinesInBlocks. The equivalent rule in seems to be padded-blocks in ESLINT. However, there is no way to specify a minimum number of lines required before the rule takes affect. So here is the problem:

Using JSCS, I can specify `"requirePaddingNewlinesInBlocks": 1" and the following are valid for this rule:

function foo() { // valid since more than 1 line between the blocks is required to throw an error
  return bar();
}
function foo() { // valid

  dobar();
  return bar();

}

Using ESLINT with the rule "padded-blocks": [2, "always"]

function foo() { // not valid according to ESLINT
  return bar();
}
function foo() { // valid

  dobar();
  return bar();

}

What does the rule currently do for this code?
The rule currently causes a linting error whenever padding is not around the blocks. So I would like to specify a rule like this:

"padded-blocks": [2, "always"]

So that we have an equivalent for the JSCS rule. Note: PolyJuice currently translate the rule as "padded-blocks": [2, "always"].

What will the rule do after it's changed?

In the example above, the linting error for the ESLINT example will be removed.

@eslintbot eslintbot added the triage An ESLint team member will look at this issue soon label Dec 15, 2016
@platinumazure platinumazure 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 Dec 15, 2016
@platinumazure
Copy link
Member

(Noting that this is related to, but hopefully not a duplicate of, #7356. I had recommended in chat that this could probably be done as an atomic proposal rather than waiting for #7356 to get moving. If anyone on the core team disagrees, I apologize for causing any confusion.)

Hi @sscaff1, thanks for the issue.

I think this behavior should definitely be behind a new option. Maybe something like this? "padded-blocks": [2, { "withMoreLinesThan": 1 }] (But please let's find a better option name)

@platinumazure
Copy link
Member

Oh, and I'll champion this.

@not-an-aardvark
Copy link
Member

How about minLines?

@cookiengineer
Copy link

cookiengineer commented Dec 20, 2016

Yes, I would prefer minLines as an option, too.

Our codestyle allows less than three statements (blocks) to require no newline in between. If the statements amount is greater than 2, then a newline is required before and after.

// this should be possible with `minLines: 3` from @not-an-aardvark

let whatever = doSomething();
if (whatever === true) {
    whatever.another();
    something_else();
}

let another = doSomething();
if (another === false) {

    something_else();
    dowhatever();
    anotherstatement();

}

@platinumazure
Copy link
Member

Okay, I think we agree that minLines is a sensible option name.

Current status: I've volunteered to champion this rule. We need 3 more team members to endorse (:+1:) the original proposal and then we can accept the issue. Then someone can write a PR. (Someone can choose to write a PR earlier, there's just the [hopefully small] risk we won't accept the issue and then the PR would be a waste of time, although the code could certainly be repurposed into a plugin.)

@jmullo
Copy link

jmullo commented Feb 10, 2017

Very much needed.

@ashclarke
Copy link

ashclarke commented Jun 21, 2017

If possible, I'd like the capability to require 1 new line padding if a block is multi-line:

// valid
if (this.thereIsThisOption &&
    this.fairlyLongConditionToo()) {
↵
    this.doSomethingElse();

    this.somethingHereToo = false;
}

// valid
if (this.thereIsThisOption &&
    this.fairlyLongConditionToo()) {
↵
    // This does something else, because reasons.
    this.doSomethingElse();

    this.somethingHereToo = false;
}

// invalid
if (this.thereIsThisOption &&
    this.fairlyLongConditionToo()) {
    // This does something else, because reasons.
    this.doSomethingElse();

    this.somethingHereToo = false;
}

// invalid
if (this.thereIsThisOption &&
    this.fairlyLongConditionToo()) {
    this.doSomethingElse();

    this.somethingHereToo = false;
}

I was thinking I could achieve it with padding-line-between-statements, but no luck as I think the multiline-block-like statement type set for prev applies to the statement after the closing brace.

I don't know if something like multiline-block-open (or multiline-block-opener) could be classified as a statement, but { blankLine: "always", prev: "multiline-block-open", next: "*" } would also solve that.

@platinumazure
Copy link
Member

@eslint/eslint-team Looks like this needs one more 👍 to be accepted! Otherwise, we should probably close this soon.

@aladdin-add
Copy link
Member

just make sure: would it be conflict with this proposal? it seems have been accpeted.

@platinumazure
Copy link
Member

Great catch @aladdin-add, I think you're right. This might conflict with that other proposal.

@sscaff1 Does the proposal here potentially meet your needs?

@sscaff1
Copy link
Author

sscaff1 commented Jun 4, 2018

Yes it does - thank you

@platinumazure
Copy link
Member

Thanks! I'll go ahead and close this issue then.

@eslint-deprecated eslint-deprecated bot locked and limited conversation to collaborators Dec 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 Dec 3, 2018
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
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 evaluating The team will evaluate this issue to decide whether it meets the criteria for inclusion rule Relates to ESLint's core rules
Projects
None yet
Development

No branches or pull requests

8 participants