Fixes for requirePaddingNewLinesBeforeLineComments (consecutive and in blocks) #1102
Conversation
| return; | ||
| } | ||
|
|
||
| if (prevToken.type === 'Punctuator' && prevToken.value === '{') { |
There was a problem hiding this comment.
This should be optional. allExcept: 'firstInBlock.
There was a problem hiding this comment.
Also, this isn't really correct as it allows inside Objects, which aren't blocks, but I suppose it's fine for now.
There was a problem hiding this comment.
I'm thinking that since we haven't rolled this rule out yet, we aren't breaking backwards compatibility. I don't think I've met anyone who would expect this rule to force:
if (true) {
// comment
}While I could totally see it being optional in the future and adding more options, I think it is fine to leave it like this for the initial release.
There was a problem hiding this comment.
Hmm, your comment on the other PR implies that jQuery would expect the form I put on my last comment. Interesting...
There was a problem hiding this comment.
Yeah, that's why I mentioned it :)
There was a problem hiding this comment.
How do we decide on the default behavior here?
It seems like if this rule is used 99% of the time with allExcept: 'firstInBlock'then that should be the default. But it would be speculation on both of our sides to try to guess which would be the 'more common' case.
There was a problem hiding this comment.
Default should be the version without exceptions. Principle of least surprise.
There was a problem hiding this comment.
@hshoff: Can you weigh in here? Would Airbnb want an override that wouldn't require a blank line before a comment if it is the first in the block?
Aka, should this be an option at all or just always require a blank line?
There was a problem hiding this comment.
@TheSavior in Airbnb land:
// bad
if (true) {
// comment
}
// good
if (true) {
// comment
}
|
@TheSavior, thanks again for contributing! |
|
Alrighty. An additional configuration option has been added. Can you guys review my doc block? I copied it from another rule but I want to make sure that it will be parsed correctly and is in the correct style. |
|
|
| } | ||
|
|
||
| if (allowFirstInBlock && prevToken.type === 'Punctuator' && prevToken.value === '{') { | ||
| console.log('skip'); |
…ents together. Allowing no padding if first line in block. requirePaddingNewLinesBeforeLineComments: Adding an optional configuration for allExcept: firstInBlock
elicwhite
force-pushed
the
fix/newLinesBeforeComments
branch
from
751436b
to
d6e8abc
Compare
|
Bump? |
|
@TheSavior no reason to bump. This will land before we release 1.12 regardless of when it actually makes it in |
|
Ah, gotcha. The flow of when things get merged in on JSCS is just a bit different than what I would expect. Still getting used to it. :) I was figuring that I would update #1103 after this and #1101 were merged, perhaps I should update it now so it will be ready for you to merge them all at the same time. |
|
Thanks again! |
requirePaddingNewLinesBeforeLineComments: Allowing multiple line comments together. Allowing no padding if first line in block.