requireCurlyBraces one-statement / one-expression exception

[kangax]

"requireCurlyBraces" is a great option but needs an exception. The only time it's acceptable to ignore it is for early returns or continuations.

Example from our Fabric.js:

 if (!target) return;

Or with continue:

if (!this._objects[i] || !this._objects[i].active) continue;

Example from Underscore:

if (iterator.call(context, obj[i], i, obj) === breaker) return;

Example from Angular:

if (key.charAt(0) === '$' || isFunction(o1[key])) continue;

etc.

Most of the time, I want if statements to have curly braces, except when there's 1 statement in its body, such as return or continue.

I couldn't find a way for JSCS to ignore these, hence the request.

I could think of 2 levels of allowance:

• Only allow "empty" statements, such as return;, continue;, break;
• Only allow one statement, such as return false;, return foo();, etc.

I'm leaning towards 2nd. I noticed that both Underscore and Angular use the latter as well.

[dwick]

+1

[Raynos]

I would want to use requireCurlyBraces that does not have this exception.

This exception should not be hardcoded but should be togglable

[rwaldron]

@Raynos +1

[ljharb]

+1 on both settings. I never want to see blocks without curly braces, but I heartily support the both mine and the OP's right to configure it.

[markelog]

Help me understand this, if don't use braces, you only allowed to use one statement, so if return foo(); is valid, what should be invalid?

foo();? Or foo() && bar();?

or this

if (key.charAt(0) === '$' || isFunction(o1[key])) continue;

should be valid, but this isn't:

if (key.charAt(0) === '$' || isFunction(o1[key])) 
    continue;

?

[kangax]

foo();? Or foo() && bar();?

Depends on what this additional option will allow for.

As far as newlines — I don't have a preference. None of the bracketless statements have newlines in my code.

Just checked Fabric. Out of ~11,6K LOC, we have:

• 63 x if (...) return;
• 10 x if (...) continue;
• 2 x if (...) break;
• 13 x if (...) return ...
  • 3 x if (...) return false;
  • 2 x if (...) return this;
  • 4 x if (...) return ''
  • 1 x if (...) return null
  • 1 x if (...) return fabric
  • 1 x if (...) return oStyle

I'm ok with only allowing plain return, break, continue.

[mikesherov]

JS as a language only allows single statements to not have curly braces. Asking for the option to allow single statements to not have curlies is the same as not using this rule at all.

Now, if the conversation is to only allow certain statements to not have curlies, I can support that.

I'd prefer to let the user specify exact statements to allow, so that we're not back here in a month asking for more exceptions.

Sound fair?

[kangax]

Yes, having some kind of whitelist of statements/expressions would work for me. Not sure how exactly you'd want to do it but, for example, I could use something like this in Fabric (see previous comment):

allowStatementsWithoutCurly: [ 'ReturnStatement', 'BreakStatement', 'ContinueStatement' ]

but then... what about some more granularity, allowing only specific expressions as part of return statement?

allowExpressionsWithoutCurly: [ 'BooleanLiteral', 'StringLiteral', 'ReservedWord' ]

I'm not sure. This could quickly become hairy. Any ideas?

[donaldpipowitch]

Maybe something like "requireCurlyBraces": [ "for", "while" ] to always require curly braces or "requireCurlyBracesInMultiLineBlocks": [ "if", "else" ] for just multi line blocks? (See here)

[kangax]

I would really prefer blacklist rather than whitelist. It would be tedious to list all statements except 1 or 2.

Sent from my iPhone

On May 15, 2014, at 8:27, Donald Pipowitch notifications@github.com wrote:

Maybe something like "requireCurlyBraces": [ "for", "while" ] to always require curly braces or "requireCurlyBracesInMultiLineBlocks": [ "if", "else" ] for just multi line blocks? (See here)

—
Reply to this email directly or view it on GitHub.

[Kienz]

+1

[bajtos]

Is there any solution yet?

I'd like to require curly braces in general, but allow else if without extra braces. The following code is rejected at the moment:

if (...) {
} else if (...) {
} else {
}

[mikesherov]

@bajtos that sounds like a bug. Can you please file a new issue for that? Thanks for contributing!

[bajtos]

@mikesherov IMO that is not a bug, as it was printed when my config included "requireCurlyBraces": [ "else"]. Removing else from the list fixed the warning.

I find the behaviour perfectly correct in the context of the current implementation: else keyword requires curly braces, therefore the correct version is this:

if (...) {
} else {
  if (...) {
  } else {
  }
}

The problem will go away once we allow keywords to bypass the CurlyBraces and include "if" in the list of white-listed keywords. That is exactly the point of this GH issue if I understand the discussion here correctly.

However, if you think that else if should be treated differently and always bypass requireCurlyBraces, then I can fill a new issue for that, perhaps even submit a pull request.

[mikesherov]

On Thursday, November 27, 2014, Miroslav Bajtoš notifications@github.com
wrote:

@mikesherov https://github.com/mikesherov IMO that is not a bug, as it
was printed when my config included "requireCurlyBraces": [ "else"].
Removing else from the list fixed the warning.

I find the behaviour perfectly correct in the context of the current
implementation: else keyword requires curly braces, therefore the correct
version is this:

if (...) {
} else {
if (...) {
} else {
}
}

The problem will go away once we allow keywords to bypass the CurlyBraces
and include "if" in the list of white-listed keywords. That is exactly the
point of this GH issue if I understand the discussion here correctly.

However, if you think that else if should be treated differently and
always bypass requireCurlyBraces, then I can fill a new issue for that,
perhaps even submit a pull request.

Yes, I do think it should be treated differently. A pull request would be
great.

—
Reply to this email directly or view it on GitHub
#244 (comment).

Mike Sherov

[mikesherov]

@bajtos please file and new bug for that issue you reported.

[bajtos]

@mikesherov done: #799

As I was trying to reproduce the problem, I have found that if else is handled correctly indeed. However, the error message printed for a missing curly brace after else includes wrong line number, which confused me. See #799 for more details.

[mikesherov]

@kangax just pinging you here as I know this is an old issue at this point but I really can't figure out a good way to specify exceptions here. Have you any thoughts about it since the last time this topic was discussed?

[kangax]

I guess whatever I brought up in #244 (comment) would be good for me. Although I ended up just adding {, } around "empty" return statements. I don't like it but whatever works, so I'm using that for the past few months.

[LukeGT]

I would like to see the "require curly braces only if the statement spans multiple lines" approach taken on board. The "requireTrailingComma" option has a similar behaviour, where you can ask only to require a trailing comma if the expression spans more than one line (or has more than one value).

In combination with a horizontal character limit, I feel like this is good enough to stop the absence of curly braces being abused while still allowing for it to make simple statements clearer and more concise.

[qfox]

if (x < 5) x = 5, continue; // ?

[markelog]

So, is there any takers for this one?

[qfox]

@markelog That's not so big issue but better to clarify what we really need and how to configure it.

Options, as I see:

• "exceptSameline";
• { "exceptSingleStatement": true || [ 'ReturnStatement', 'BreakStatement', ... ] };
• { "exceptExpressions": [ 'BooleanLiteral', 'StringLiteral', 'ReservedWord', ... ] }.

Not sure ;-( Can we discuss this?

[markelog]

Only allow one statement, such as return false;, return foo();, etc.

This sounds good to me

[hzoo]

Anyone want to summarize what is wanted here? Sounds like we can make an option allExcept`: ['ReturnStatement', 'BreakStatement', 'ContinueStatement']

[kangax]

That would work for me.

Sent from my iPhone

On 02 Oct 2015, at 21:29, Henry Zhu notifications@github.com wrote:

Anyone want to summarize what is wanted here? Sounds like we can make an option allExcept: ['ReturnStatement', 'BreakStatement', 'ContinueStatement']`

—
Reply to this email directly or view it on GitHub.

[markelog]

Cool!

[hzoo]

Ok need to change it a bit more since the original option was an array so something like

"requireCurlyBraces": {
  "keywords": ["if", "else"], 
  "allExcept" : ['ReturnStatement', 'BreakStatement', 'ContinueStatement']`
}

[markelog]

"keywords": ["if", "else"], 
"allExcept" : ['ReturnStatement', 'BreakStatement', 'ContinueStatement']`

Seems inconsistent, this rule already maps those entities together, so it should be pretty easy to use same type of lists.

Generally it is good idea to distance from parser json

[hzoo]

What do you mean by this? combine them into a single list or either keywords or allExcept?

[markelog]

I meant if keywords has "token list", then it would be logical for allExcept to have the same, then AST entities

[hzoo]

Oh ok you mean like "return", "break", "continue"

[markelog]

yeah

[kangax]

Fantastic, thank you!