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
add outer padding to rule padded-blocks and cover existing jscs functionality #7356
Comments
Closing the other issues so we can consolidate the discussion in one issue. Thanks @quinn! @eslint/eslint-team Can we get some opinions from team members? @TheSavior and @quinn have done a great job of pushing forward with this on their own but I think we need the team involved. |
@kaicataldo Anything I can do to help? I would be happy to try an implementation but there are already multiple PRs and I don't want to redo anyone else's work. I could try to merge the existing PRs or something. If there's anything I can do let me know. thanks :) @kaicataldo Maybe the discussion from this issue should be moved here? #7145 In my example above, I only included the "singleLine" exception for "outer" padding, not inner, since it was not part of the current "padded-blocks" / jscs implementation. "singleLine" I think it a little bit odd in that list because it's slightly different than the other options (all of the other options have to do with the location of the block rather than the structure of the block). idk hopefully i am not making things more complicated :p |
@quinn Sorry for not responding sooner - I think it's fine to leave that as a separate discussion so we can focus on padding outside blocks. Hopefully we'll get some discussion on here soon - thanks for your patience. |
Thanks for the proposal. This looks good, but I have a few questions/comments:
|
|
Maybe it's just me, but this seems like too much to combine into |
@nzakas The issue is that the two rules would conflict with each other: /* eslint padded-blocks: [error, "always"] */
/* eslint padding-outside-blocks: [error, "never"] */
{
function foo() {
} // should there be an empty line below? padded-blocks says yes, the other rule says no
} |
@nzakas @not-an-aardvark yeah. that conflict is one of the causes of the lengthy discussion. keeping it as a separate rule was discussed, but I think someone indicated it would be more weird to have a rule behave differently based on the.. rules of another rule. thats the reason for the |
What about changing concept totally and split the rule in:
In this way there would be no conflict, as far as I can see. |
To further elaborate, reading the JSCS rule in question makes it clear thet it's a quite different subject, only regarding padding after blocks. My proposal does not cover the block-statement boundaries but only the block-block boundaries. I think a new small focused rule is better than a large number of parameters for an already existing rule. What do you think? |
@caesarsol I think the only reason all of this is being considered as a single rule is because of the situations where the 2 rules could conflict and there isn't a precedent in eslint already for what to do when 2 rules say to a different thing for a given statement (afaik) does that help or did I miss the point of what you were saying ?? |
I'm proposing about avoiding the conflict! Maybe with a new concept of block boundaries. |
@caesorsol ok, the issue isn't between blocks, it's when a block has inside of it a block as the first or last thing.. does that make sense? Also, I personally want padding around my blocks even if the things on either side are not blocks (except in the case of a wrapping block..) |
(Sorry, it was difficult for me to explain)
For each of those a rule about enforcing the newline. This would certainly leave the inner block padding alone, making space for the rule without conflicts. Any downside you can think of? |
Many of our rules that enforce blank newlines before and after nodes defer to If we keep this as two separate rules - one that enforces padding inside blocks and one that enforces padding outside blocks - could we not do the same thing? JSCS has both these rules - how did they handle cases where they conflict? |
Looking quickly at the 4 JSCS rules' source, seems there isn't any rules-conflict detection... |
I believe JSCS relied on users to not create conflicting configurations. |
lets say that the rules you haven't defined enforce padding before and after a block, and also enforce no padding at the beginning and end of the inside of a block. the "conflict" here happens between lines 1 and 2, and between lines 8 and 9. I don't think there is a need to address redundant padding between blocks since I think this can be intuitively collapsed much like margin collapsing in CSS. Sorry if this is redundant, just want to confirm that we're all on the same page with this and these things imo can be hard to discuss w/o example code.. |
@quinn, something we ran into with the JSCS rule are things like functions that are wrapped with bind, call, or apply. There should be a way to enforce a blank line after blocks wrapped in those, unless they come at the end of a parent block in which case there shouldn't be an extra blank line. Does that make sense? Take a look at the jscs rule docs if not |
I'm sorry for late. This is a abstract rule for linebreaks between statements, just it complements {
// padded-blocks
foo()
// newline-between-statements
bar()
// newline-between-statements
{
// padded-blocks
foo()
// newline-between-statements
bar()
// padded-blocks
}
// newline-between-statements
baz()
// padded-blocks
}
// eol-last The point of this rule is that this rule focuses "between 2 statements" rather than "before/after a statement". This rule has flexible configuration, so it can replace One of cons is that users may be relatively hard to configure it. Thought? |
I've probably missed something but I can't see @kaicataldo how you could later extend this rule by having newline-after-block: "error", "always" or {
FunctionDeclaration: "always",
IfStatement: "never"
} without conflicting with rules like Unless I'm missing something (and I probably am) the only way to not have conflicts is to have a config more like newline-after-block: [FunctionDeclaration, IfStatement],
newline-before-block: [ReturnStatement] |
@mysticatea As you said, this rule will be hard to configure. The fact that configuring the rule would require knowledge of the different AST node types is a smell of that. To me, it's an example of an advanced rule that could also exist as custom rule for people wanting total control over line padding, but I would also like to hear more opinions. |
Had a response written out on my phone and lost it - sorry! @mysticatea I have the same concern as @alberto about the configuration being complex and requiring knowledge of the AST. That adds a lot of overhead to configuring a pretty universal rule, and I don't think it's necessarily fair to force people to have to learn this stuff to use the rule. I'm also curious about some node type-specific quirks, like not requiring a newline before a I do like the concept of the proposal, though. Is there any way we could make the configuration simpler/easier to understand for someone who knows JS but might not have prior experience with JS ASTs? I think I could support this proposal if we can find a way to do so, as it does give more control. @aboyton The reason we can extend this is that we discussed deferring to other rules (padded-blocks, newline-before-return, and lines-around-comment, and newline-after-var) when those situations are encountered. There's precedence for this in our other rules. @mysticatea's proposal would combine all these into one rule so we wouldn't need to do that. |
I agree that it would be relatively hard to configure. But I think it's hard as well if somebody configures multiple rules for newline carefully as avoiding conflictions. "Rules for newline after statement" and "rules for newline before statement" are conflictable essentially. We would need to set priority but the priority would be unclear and not controllable. Or we would need to add options but it might get the combinatorial explosion. On the other hand, Also,
In addition, we have some rules which use AST node types already. E.g. no-multi-spaces.
Because if (a) {
// padded-blocks
return
// padded-blocks
}
// newline-between-statements
if (b) {
// padded-blocks
foo()
// newline-between-statements
return
// padded-blocks
} However, yes, my proposal has 2 special types: |
The strategy we've been discussing to avoid conflicts is to defer to rules that enforce
I agree that most of these names are self-explanatory. Maybe it wouldn't be too much overhead? I really do like the concept of changing this from looking at nodes individually to looking at statements in pairs. |
@kaicataldo @mysticatea the |
@mysticatea's proposal changes the original concept of enforcing padding lines around nodes to checking between nodes - so you define relationships, with priority being set by the order. If we go this route, we would be able to deprecate newline-after-var, lines-around-directive, and newline-before-return, leaving exceptions for lines-around-comment and padded-blocks. The proposal I outlined above would require that the existing rules be kept. @eslint/eslint-team Do we have any other opinions on this? My goal here is implementation and JSCS compatibility - let's try to decide on something! For reference, the two proposals on the table right now are #7356 (comment) and #7356 (comment). I'm personally leaning toward @mysticatea's suggestion right now - my biggest concern is configuration complexity. |
oh, neat. I like @mysticatea 's idea a lot ! |
@mysticatea can you show how you would configure the exceptions from the original JSCS rule? |
TSC Summary: There are currently two proposals:
TSC Question: Which rule should we accept, if any? |
Ah, JSCS I think newline rules of each place should check it, probably behind options, if it's needed. Because even if we make an independent rule, we need
For Maybe we can use the keyword which is at the top of statements instead of AST node types.
|
@mysticatea I really like the direction your proposal is going. Could we support both cases (node types and keywords)? And what about groupings (e.g., maybe all Export*Declaration can have no blank lines in between but there must be a blank line surrounding the group)? |
Sure.
It can realize with:
|
TSC resolution: We have decided to go with @mysticatea proposal. @mysticatea are you championing this? |
Also thanks to everyone who contributed in the discussion or with a PR and helped us flesh this out! special hat tip to @quinn @TheSavior @LinusU @ekmartin We know it has been painfully slow, and we apologize for that |
While this configuration enables lots of flexibility, it's also going to put a higher priority on solving the problem about extending configurations: #7957 Since all of the rules are being consolidated, if, for example, airbnb implements this, nobody will be able to add more things without having to duplicate their config for this rule and thus not be able to stay in sync. |
* New: newline-between-statements (fixes #7356) * Chore: refactor with AST selectors * Fix: add more tests. - The kind `directive` matches to only directive prologue. - The kind `expression` does not match to directive prologue. - The kinds `block-like` and `multiline-block-like` match to do-while statements. - The kinds `block-like` and `multiline-block-like` do not match to classes. * Update: change options' form. - It was the array of arrays, but it becomes the array of objects. * Update: rename * Update: deprecate 3 rules as replaced by this * use astUtils.STATEMENT_LIST_PARENTS instead of local RegExp. * avoid catastrophic backtracking * improve IIFE check for unary expressions * fix a bug about semicolon-less style and empty statements * unwrap the array of options * use constants for error messages. * update document * fix plural * fix blankline → blankLine
Update: please refer to this pull request for the current status of this issue:
#8099
1st proposal: updating padded-blocks
The above doesn't yet include additional options for backwards compatibility.
related issues:
related / converging rules:
2nd proposalno modifications topadded-blocks
. new rule that has exceptions to allowpadded-blocks
to work, if there is a conflict an error is thrown (is this even a thing?)"afterOpeningBlock"
and"beforeEndingBlock"
specify whether the rule should apply to a block placed at the beginning or end of the inside of another block. If they are both specified, thenpadded-blocks
should work as expected. If they are not specified, then the new rule could potentially result in an error depending howpadded-blocks
is configured.The text was updated successfully, but these errors were encountered: