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

Rule Proposal: nonblock-body-position #6067

Closed
mysticatea opened this issue May 4, 2016 · 13 comments · Fixed by singapore/lint-condo#244 or renovatebot/renovate#123 · May be fixed by iamhunter/teammates#4
Closed

Rule Proposal: nonblock-body-position #6067

mysticatea opened this issue May 4, 2016 · 13 comments · Fixed by singapore/lint-condo#244 or renovatebot/renovate#123 · May be fixed by iamhunter/teammates#4
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 feature This change adds a new feature to ESLint rule Relates to ESLint's core rules

Comments

@mysticatea
Copy link
Member

mysticatea commented May 4, 2016

From requireNewlineBeforeSingleStatementsInIf.

This rule will enforce the position of bare statements at the body of if, else, do-while',while,for,for-in, orfor-of`.

{
    "nonblock-body-position": ["error",  "below" or "beside" or "any"],
    // or
    "nonblock-body-position": ["error", {
        "position": "below" or "beside" or "any",
        "overrides": {
            "if": null,
            "else": null,
            "do-while": null,
            "while": null,
            "for": null,
            "for-in": null,
            "for-of": null
        }
    }]
}
  • position
    • "below" (default) - Requires newline before the body's statement.
    • "beside" - Disallow newline before the body's statement.
    • "any" - Does not enforce the position.
  • overrides - People can override setting for each kind.

Valid:

/*eslint nonblock-body-position: ["error", "below"]*/

if (a)
    foo();
else
    foo();

while (a)
    foo();

// ignore block statements (those are waned by brace-style)
if (a) { foo(); }
if (a) {
    foo();
}
/*eslint nonblock-body-position: ["error", "beside"]*/

if (a) foo();
else foo();

while (a) foo();

// ignore block statements (those are waned by brace-style)
if (a) { foo(); }
if (a) {
    foo();
}
/*eslint nonblock-body-position: ["error", {"position": "beside", "overrides": {"while": {"position": "below"}}}]*/

if (a) foo();
else foo();

while (a) 
    foo();

Invalid:

/*eslint nonblock-body-position: ["error", "below"]*/

if (a) foo();
else foo();

while (a) foo();
/*eslint nonblock-body-position: ["error", "beside"]*/

if (a)
    foo();
else
    foo();

while (a)
    foo();
/*eslint nonblock-body-position: ["error", {"position": "beside", "overrides": {"while": {"position": "below"}}}]*/

if (a)
    foo();
else
    foo();

while (a) foo();
@mysticatea mysticatea added rule Relates to ESLint's core rules feature This change adds a new feature to ESLint evaluating The team will evaluate this issue to decide whether it meets the criteria for inclusion labels May 4, 2016
@mysticatea mysticatea added this to the JSCS Compatibility milestone May 4, 2016
@ilyavolodin
Copy link
Member

I think this rule is going to conflict with brace-style allowSingleLine option.

@mysticatea
Copy link
Member Author

mysticatea commented May 4, 2016

I tried it, but brace-style does not check non-block statements since it's brace-style.

@danielcavanagh
Copy link

the other problem with allowSingleLine is that it can't enforce the use of a single line, so --fix does nothing

it seems that positioning of the single line, whether braced or not, should be part of this one rule (perhaps renamed to single-statement-body-position?) and the ruling on whether to brace should be left to curly (which should probably also be renamed...)

@mysticatea
Copy link
Member Author

Thank you @danielcavanagh .
I had misunderstood it. I have read tests of requireNewlineBeforeSingleStatementsInIf, then it's warning statements in a block as well.

Also, original rule is warning the first statement of a block which has multiple statements as well.

@mysticatea mysticatea changed the title Rule Proposal: nonblock-body-style Rule Proposal: nonblock-body-position May 19, 2016
@mysticatea
Copy link
Member Author

mysticatea commented May 19, 2016

So, the intention of original JSCS rule is "Disallow that statements locate at beside if/else.".

For JSCS compatibility:

  • We can use {allowSingleLine: false} option of brace-style rule if there is a block.
  • We can use this rule if there is not a block.

@alberto alberto added accepted There is consensus among the team that this change meets the criteria for inclusion help wanted The team would welcome a contribution from the community for this issue and removed evaluating The team will evaluate this issue to decide whether it meets the criteria for inclusion labels Feb 17, 2017
@not-an-aardvark not-an-aardvark self-assigned this Feb 19, 2017
@kaicataldo
Copy link
Member

kaicataldo commented Feb 20, 2017

How does this relate to #8099 now that we have that hammered out?

We're consolidating some other existing rules that enforce padded newlines around nodes into that rule - seems like this could introduce a new conflict.

Should this maybe be covered by that rule as well?

@not-an-aardvark
Copy link
Member

As far as I can tell, this addresses a separate issue from #8099. #8099 addresses newlines between consecutive statements, whereas this addresses newlines between nested statement nodes. For example, consider the following newline-between-statements configuration:

/* eslint newline-between-statements: ["error", ["blankline", "if", "return"]] */

if (foo)
  return 1; // newline-between-statements *does not* enforce a newline before this statement.

return 2; // newline-between-statements *does* enforce a newline before this statement.

@kaicataldo
Copy link
Member

@not-an-aardvark Thanks, that's a really helpful distinction. Makes sense!

@kaicataldo
Copy link
Member

kaicataldo commented Feb 20, 2017

Sorry to respond to this so late in the game, but I also wonder if we can decide on a name that more clearly describes what this rule is checking. nonblock-body-position wasn't immediately clear to me, so I can imagine it would be confusing to someone who's not a project maintainer. I know it's long, but I think nonblock-statement-body-position would be clearer (hoping we can come up with a shorter/clearer name than that!).

@mysticatea
Copy link
Member Author

nonblock-statement-body-position makes sense to me.

I look at this again, I wonder if nonblock-statement-body-position should include the body of arrow expression as well, or not.

() => foo()
// or
() =>
    foo()

@kaicataldo
Copy link
Member

@mysticatea Thinking about this more, I wonder if we shouldn't include ArrowFunctionExpressions. Seems like it's the odd one out, since the rest of the nodes it checks are statements. Could be confusing, particularly if we change the name to include statements. I think I'm actually 👎 to including them.

@platinumazure
Copy link
Member

We can always cover arrow function expressions in a separate rule. I think we should focus on statements in this one.

@mysticatea
Copy link
Member Author

About including arrow expressions seems unpopular in team, so let's go as is.

@alberto alberto moved this from Todo to Done in JSCS Compatibility Mar 13, 2017
@alberto alberto removed the help wanted The team would welcome a contribution from the community for this issue label Nov 11, 2017
@eslint-deprecated eslint-deprecated bot locked and limited conversation to collaborators Feb 6, 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 Feb 6, 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 feature This change adds a new feature to ESLint rule Relates to ESLint's core rules
Projects
No open projects
7 participants