Skip to content
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 "multiline-const", "multiline-let" and "multiline-var" STATEMENT_TYPEs to padding-line-between-statements rule #10721

Closed
neemzy opened this issue Aug 2, 2018 · 17 comments
Assignees
Labels
accepted There is consensus among the team that this change meets the criteria for inclusion archived due to age This issue has been archived; please open a new issue for any further discussion enhancement This change enhances an existing feature of ESLint rule Relates to ESLint's core rules

Comments

@neemzy
Copy link
Contributor

neemzy commented Aug 2, 2018

What rule do you want to change? padding-line-between-statements

Does this change cause the rule to produce more or fewer warnings? More

How will the change be implemented? (New option, new default behavior, etc.)? New accepted values for prev and next configuration options

Please provide some example code that this change will affect:

const bouya = { hey: "ho" };
const kasha = {
  tik: "tok",
  onda: "clock"
};

What does the rule currently do for this code? The rule is unable to differentiate single-line and multi-line const, let and var statements.

What will the rule do after it's changed? The rule will offer the possibility to treat aforementioned cases differently, e.g. enforce blank lines around a multi-line variable declaration but not around a single-line one.

@eslint-deprecated eslint-deprecated bot added the triage An ESLint team member will look at this issue soon label Aug 2, 2018
@aladdin-add aladdin-add added enhancement This change enhances an existing feature of ESLint rule Relates to ESLint's core rules evaluating The team will evaluate this issue to decide whether it meets the criteria for inclusion and removed triage An ESLint team member will look at this issue soon labels Oct 5, 2018
@platinumazure platinumazure self-assigned this Oct 5, 2018
@platinumazure
Copy link
Member

I'll champion this.

@eslint/eslint-team Can we get some 👍s on this please?

@neemzy
Copy link
Contributor Author

neemzy commented Oct 30, 2018

@platinumazure Any news on this? :)

@platinumazure
Copy link
Member

@neemzy Thanks for following up. Trying to muster support again 😄 Hoping this might be accepted in a few days, otherwise we'll have to close per our issue backlog policy.

@platinumazure
Copy link
Member

@mysticatea @kaicataldo @not-an-aardvark Any thoughts on this proposal?

@not-an-aardvark
Copy link
Member

Is the intent that this should change the behavior of the existing var, let, and const STATEMENT_TYPEs to only match single-line statements? Based on skimming #10919 it seems like that is what's been implemented, but it seems like that would be a breaking change for users who rely on var, let, and const matching multiline statements.

@platinumazure
Copy link
Member

Thanks @not-an-aardvark, I didn't see the PR.

I would consider the PR as breaking at this point. I think we should support the multiline-only case in a semver-minor release and change the meaning of the existing types in a semver-major.

@not-an-aardvark
Copy link
Member

not-an-aardvark commented Oct 30, 2018

To me, it seems confusing to have a type called var that actually means "single-line var statement" (even if we do it after a breaking change). What if we added something like singleline-var and kept var as-is?

@neemzy
Copy link
Contributor Author

neemzy commented Nov 2, 2018

So var would match both single-line and multiline statements, and singleline-var and multiline-var would be added to allow for more specific targetting? That would be fine by me and would indeed avoid introducing a BC break. @platinumazure @not-an-aardvark Do you want me to update the PR accordingly?

@platinumazure
Copy link
Member

@neemzy Please do, thanks!

@neemzy
Copy link
Contributor Author

neemzy commented Nov 5, 2018

@platinumazure Should be good! I also added back the tests I had removed since they are passing again this way.

@platinumazure
Copy link
Member

@not-an-aardvark Given that the proposal and PR have both been tweaked to address your concerns, are you 👍 to this proposal now?

@kaicataldo @ilyavolodin Any thoughts on this proposal?

@kaicataldo
Copy link
Member

Sorry, I seemed to have missed this. I'm generally in favor of this proposal, but I do think it's a bit confusing to be consistent between expression/multiline-expression and var/singleline-var/multiline-var, where expression is for a single-line expression and multiline-expression is for a multiline expression, whereas var is for both single-line and multiline var declarations, singleline-var is for only for single-line var declarations, and multiline-var is for multiline var declarations.

Also, what is the expected behavior when there is a conflict between, say, vars and singleline-vars?

@platinumazure
Copy link
Member

platinumazure commented Nov 13, 2018

Thanks @kaicataldo for your feedback!

Sorry, I seemed to have missed this. I'm generally in favor of this proposal, but I do think it's a bit confusing to be consistent between expression/multiline-expression and var/singleline-var/multiline-var, where expression is for a single-line expression and multiline-expression is for a multiline expression, whereas var is for both single-line and multiline var declarations, singleline-var is for only for single-line var declarations, and multiline-var is for multiline var declarations.

I agree that would be confusing, but I don't think that's the case.

From the current version of the rule:

    expression: {
        test: (node, sourceCode) =>
            node.type === "ExpressionStatement" &&
            !isDirectivePrologue(node, sourceCode)
    }

So this proposal maintains consistency: var is any variable declaration, multiline-var is only multiple-line variable declaration statements, and singleline-vars would be only single-line variable declarations. (We don't currently have a singleline-expression, though.)

Also, what is the expected behavior when there is a conflict between, say, vars and singleline-vars?

Good question.

It would be nice if we could say multi or single line always wins out as it's more specific. However, we could treat these the same as how the rule treats any other conflict: "If a statement pair matches multiple configurations, the last matched configuration will be used."

So we could optionally encourage users to specify the generic var configuration first, and then singleline-var or multiline-var second... But I think we also could just rely on the existing approach and documentation.

@kaicataldo
Copy link
Member

Thanks for answering my questions. One final question regarding the behavior of the last declared configuration winning - would a conflict between singleline-var completely override var? Or just for single-line declarations?

I think it would be probably clearest if either singleline-var or multiline-var occur after var that var is ignored entirely, not just for the cases where the two overlap. However, it seems like singleline-var and multiline-var should not treat each other in this respect, and instead just follow the behavior described above as if they're separate types. Does that make sense?

@not-an-aardvark
Copy link
Member

I think it would be probably clearest if either singleline-var or multiline-var occur after var that var is ignored entirely, not just for the cases where the two overlap.

I disagree, I think that inconsistency would have the potential to create more confusion. It seems like the statement types in padding-line-between-statements are not designed with the goal of avoiding overlap -- for example, we already have the block, block-like, and multiline-block-like statement types overlapping, as well as the * statement type which overlaps with everything else. The rule has a sound and documented way of avoiding conflicts caused by overlap (namely it use the last matching type), so I think it would be confusing if it used a different method of resolving conflicts for singleline-var and its variants.

@kaicataldo
Copy link
Member

I forgot about * - that's a great point. I retract my suggestion above. 👍'd.

@neemzy
Copy link
Contributor Author

neemzy commented Nov 15, 2018

I reckon it means my PR's current implementation is OK for you? :)

@kaicataldo kaicataldo added accepted There is consensus among the team that this change meets the criteria for inclusion and removed evaluating The team will evaluate this issue to decide whether it meets the criteria for inclusion labels Nov 15, 2018
@eslint-deprecated eslint-deprecated bot locked and limited conversation to collaborators Jun 7, 2019
@eslint-deprecated eslint-deprecated bot added the archived due to age This issue has been archived; please open a new issue for any further discussion label Jun 7, 2019
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
accepted There is consensus among the team that this change meets the criteria for inclusion archived due to age This issue has been archived; please open a new issue for any further discussion enhancement This change enhances an existing feature of ESLint rule Relates to ESLint's core rules
Projects
None yet
Development

No branches or pull requests

5 participants