Lines-around-comments: allowBlockStart has issue with return block

[gyandeeps]

Eslint: v0.24.1
Praser: default

Rule config:

{
    "lines-around-comment": [2, { 
        "beforeLineComment": true, 
        "beforeBlockComment": true, 
        "allowBlockStart": true 
    }]
}

Code:

/* eslint lines-around-comment:[2, { beforeLineComment: true, beforeBlockComment: true, allowBlockStart: true }]  */
function hi(){
    return{
        /**
        * hi
        */
        test: function() {
        }
    }
}

Actual:

$ eslint --reset index.js

index.js
  4:2  error  Expected line before comment  lines-around-comment

? 1 problem (1 error, 0 warnings)

Expected:
No errors.

[btmills]

it allows the comment to be present at the start of any block statement without any space above it.

From that description (from the docs for allowBlockStart), this sounds intentional, and object literals would be a different exception.

[xjamundx]

Took me a few times, but I think @btmills may be right. That's not really a block, it's just an object literal.

[gyandeeps]

Guys @xjamundx @btmills Any proposals?
FYI, I have removed the labels.

[btmills]

Is this now a proposal for allowObjectStart/allowObjectEnd exceptions?

[gyandeeps]

Frankly speaking I am not sure.

[mysticatea]

I believe this is a work of allowBlockStart/allowBlockEnd.

I'm not happy to add more options for every syntax, such as ObjectExpression, ArrayExpression, and ClassBody.
In fact, ClassBody has been treated by allowBlockStart/allowBlockEnd already.

[nzakas]

👎 I don't think we need more options here. It's behaving as intended.

[btmills]

I'm also 👎 unless someone comes along to say they really need this.

[gyandeeps]

closing based on the above discussion.

[Cellule]

👍 At first I thought as well the the option included object literals. Basically comments preceded by { should not need a space. In my opinion it just feels weird to write

const foo = {

  // this property is important
  important: 3.1415
};

and I would rather write

const foo = {
  // this property is important
  important: 3.1415
};

[mysticatea]

I agree with @Cellule.

My hope is that allowBlockStart and allowBlockEnd treat the object literals also, it's similar to ClassBody.

(My true hope is to remove allowBlockStart and allowBlockEnd options, and comments are allowed at the top and the last by default.)

[nzakas]

Object literals don't define blocks, so while they look the same, they are completely different.

So we should not expand the existing options to include this case. We could add two new options per @btmills suggestion.

[Cellule]

I don't particularly mind adding options, I already do for blocks, might as well do it for anything block-like

[xjamundx]

What about a generic afterCurlyBrace exception? I think that appears to be what is wanted.

[nzakas]

No. These should not be treated as the same thing (see my previous comment).

[mathieumg]

The two ways out of this seem to be:

• Add more options for every syntax, such as ObjectExpression, ArrayExpression, and ClassBody. (@mysticatea)
• A generic afterCurlyBrace exception? (@xjamundx)

I personally would have been content with either the second one, or both of them. (i.e. second one overrides former one), but it seems it's a no go for the second option as per @nzakas, so I imagine it'll be the first one? A list of node types against which to validate, a bit like in no-multi-spaces?

[drewed]

👍 I'm with @Cellule and @mysticatea on this. And while I acknowledge that an object literal is not a code block, per se, as pointed out by @btmills, @mysticatea is correct that a class body is not a block either yet it's treated by the allowBlockStart exception. It seems to me that the object literal and the class body should be treated equivalently by this rule (but that's my opinion).

It doesn't help that the rule and the exceptions use two different definitions of the term block (the former referring to block quotes where the latter refers to a code block). It might help to change the name of the exception to something along the lines of allowBraceStart or allowScopeStart though one could argue against the former if you want arrays included and the latter if you're in ES5 land.

[mathieumg]

@nzakas Has a resolution path been decided for this? (See my previous reply) Thanks!

[nzakas]

See my previous reply: #2965 (comment)

[mathieumg]

My stab at this: mathieumg@3c4df61

I didn't update the documentation or provide many tests, yet, because of some hurdles related to the discussion above. Should allowObject__ and allowArray__ options only consider proper objects/arrays or also consider destructuring using braces/brackets? If no, does it mean we need to add allowArrayPattern__ and allowObjectPattern__ as well? I feel we're in for quite a ride, if so.

The commit I linked to implements the first approach to simplify things. However, let me bring something new to the discussion: I realized that the exception case is always when the line above is less indented (with allow__Start) or the line below is less indented (with allow__End), would an option that leverages that fact be better than starting to consider all possible individual cases?

Thanks.

[mathieumg]

@nzakas What is missing to mark this as accepted? I want to help this move forward but from the lack of discussion it might mean I'm the only party interested by that change?

[drewed]

@mathieumg I'm also interested in the change but lack the experience in the eslint code base to contribute meaningfully to your implementation ideas.

[nzakas]

@mathieumg just forgot, thanks for the nudge.

[JamesMessinger]

Any update on this? It seems that everybody is in agreement about the right way to fix it, and the code has already been committed by @mathieumg. Will this fix be included in the next version of ESLint?

[mathieumg]

I still have to add tests, update the documentation and get it reviewed by collaborators. (Plus rebase my changes on top of master, they were made on ESLint 1.3) School has started again so I'm really busy with that nowadays, but perhaps I can put in some time to move this past the finish line during "reading week" in a week and a half. Of course, if it's more urgent I wouldn't mind if anyone took what I started and finished it.

[viveleroi]

I might do what I can to help push this along but doubt I can find enough time. As much I dislike +1-ing things, based on this conv I feel it's important to say I'd really appreciate this feature.