lines-around-comment ES2015 classes in new allowClassStart option

[Mardak]

Tell us about your environment

• ESLint Version: v4.0.0-alpha.2
• Node Version: v7.10.0
• npm Version: 4.5.0

What parser (default, Babel-ESLint, etc.) are you using?
default

Please show your full configuration:
--env es6 --rule lines-around-comment:["error",{allowObjectStart:true}]

What did you do? Please include the actual source code causing the issue.

class A {
  /* test */
  constructor() {}
}

What did you expect to happen?
No errors.

What actually happened? Please include the actual, raw output from ESLint.
2:3 error Expected line before comment lines-around-comment

For #2894 @gyandeeps added ES2015 classes to "allowBlockStart" via 9a2dcc2 disallowing an empty line before a comment block just within a class, but it seems like it would be more appropriate as part of its own "allowClassStart".

This proposed change would better match the breaking change in #7879 by @alberto in e232464 where classes are treated differently than block statements. In particular, if someone had --rule padded-blocks:["error","never"] in addition to the above --rule lines-around-comment:["error",{allowObjectStart:true}], neither rule would be happy with a line or without a line, but if the code was using let A = { instead of a class, it would be fine.

I would think that moving class from allowBlockStart would also be a breaking change.

[not-an-aardvark]

Thanks for the issue.

Hmm, I'm unconvinced that this is worth changing. Technically, classes are neither object literals nor blocks, but I don't see how treating them as objects is more valid than treating them as blocks. In fact, the change in #7879 that you linked has the effect of treating classes more like blocks in the padded-blocks rule, since they are treated now like blocks by default in that rule.

[ljharb]

Perhaps an allowClassStart option would be more appropriate, since class is neither a block nor an object?

[Mardak]

Updated issue and PR to move class checks as part of allowClassStart from allowBlockStart.

To be clear, we started running into this issue with v4 because padded-blocks now handles classes, which is desired, but a comment block just inside the class conflicts with lines-around-comment unless allowBlockStart is set, which would also allow lines inside actual blocks, which is undesired.

In regards to classes being different than objects or blocks, that's true, but it's relatively natural to switch from an object (for a function/constructor prototype) to a class when "upgrading" to classes.

[ilyavolodin]

I'm unconvinced that naming change is important enough to create a breaking change. While technically, classes are not blocks, they are treated as blocks by scoping. Block scoping in ES6 includes classes, so argument can be made that people might think of them as blocks. If it wouldn't be a breaking change, I wouldn't have a problem with this, though.
P.S. Last version we release was a beta, so we will try to avoid any new breaking changes until version 5.

[Mardak]

I've updated the issue and PR to not be a breaking change by not altering the behavior of allowBlockStart and instead just adds the new option allowing just class "blocks" without including other blocks.

[platinumazure]

@Mardak @not-an-aardvark @ilyavolodin @ljharb Apologies, seems we've lost track of this.

Can anyone please summarize where we're at and what our next steps should be? Thanks!

[not-an-aardvark]

To clarify, with the current proposal are classes included in both allowBlockStart and allowClassStart? If so, what happens if the options are something like {allowBlockStart: true, allowClassStart: false}?

[ljharb]

I would expect allowClassStart set to false to disable the class behavior in allowBlockStart as well - since that's a new option, it wouldn't be a breaking change.

In other words, allowBlockStart would continue to look at class, which would overlap with allowClassStart - thus allowClassStart: true, allowBlockStart: false would still check class.

In a later semver-major, perhaps, the class behavior should be removed from allowBlockStart.

[Mardak]

I updated the PR #8565 with the desired explicit allowClass{Start,End}: false behavior of overriding allowBlock: true (to act as if there were not an allowBlock: true for class "blocks") and added positive class tests for Block, Class and !Block+Class for classes and negative tests for default and Block+!Class.

[Mardak]

Does the evaluating tag need to be removed to finish up this issue?

[not-an-aardvark]

Hi, sorry about the delayed response. We generally accept enhancements only if the team reaches consensus that they should be added (by having 3 👍s from team members on the issue, and having one team member willing to champion the proposal). This is a high bar by design -- we can't realistically accept and maintain every feature request in the long term, so we only accept feature requests which are useful enough that there is consensus among the team that they're worth adding.

In this case, it seems like this issue hasn't reached consensus yet. I'll ping @eslint/eslint-team to see if anyone else is willing to support/champion this proposal. If the issue gets one more 👍 and someone willing to champion it, then we'll mark it as accepted; otherwise, unfortunately we'll have to close the issue and PR.

[kaicataldo]

Just need a champion now :)

[Mardak]

@not-an-aardvark we've been holding off on upgrading to ESLint 4 because of this, but I guess we'll just turn off rules to get things working: mozilla/activity-stream#3705

I see there's 3 👍s, and this is just looking for a champion, so anything I can do to help get this marked as accepted?

[kaicataldo]

Sorry for the delay here. I'll champion this. That being said, we need another thumb now (I can't both be a thumb and a champion!) @eslint/eslint-team

[not-an-aardvark]

👍