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

Indent Rule - Possible to add LogicalExpression for multi-line conditions? #8978

Closed
ashclarke opened this issue Jul 21, 2017 · 6 comments
Closed
Labels
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 evaluating The team will evaluate this issue to decide whether it meets the criteria for inclusion indent Relates to the `indent` rule rule Relates to ESLint's core rules

Comments

@ashclarke
Copy link

ashclarke commented Jul 21, 2017

What rule do you want to change?

Add LogicalExpression as a configuration option to the Indent rule.

I believe the default should be "off", as per this comment.

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

Code and setting dependent. I think the addition would not have an affect to default behaviour, unless explicitly set in eslintrc.json.

How will the change be implemented? (New option, new default behavior, etc.)?

New LogicalExpression option for the Indent rule configuration

Please provide some example code that this change will affect:

"indent": [2, 2, {
    "LogicalExpression": "first"
}
// Valid
if (this.condition) {

}

// Valid
if (this.condition &&
    this.otherConditionFunctionName() &&
    this.anotherConditionFunctionName()) {

}

// Valid
var isAConditionalVariable = this.condition &&
                             this.otherConditionFunctionName();

// Invalid
if (this.condition &&
  this.otherConditionFunctionName() &&
  this.anotherConditionFunctionName()) {

}

// Invalid
var isAConditionalVariable = this.condition &&
  this.otherConditionFunctionName();
"indent": [2, 2, {
    "LogicalExpression": 1
}
// Valid
if (this.condition) {

}

// Valid
if (this.condition &&
  this.otherConditionFunctionName() &&
  this.anotherConditionFunctionName()) {

}

// Valid
var isAConditionalVariable = this.condition &&
  this.otherConditionFunctionName();

// Invalid
if (this.condition &&
    this.otherConditionFunctionName() &&
    this.anotherConditionFunctionName()) {

}

// Invalid
var isAConditionalVariable = this.condition &&
                             this.otherConditionFunctionName();

Default

"indent": [2, 2, {
    "LogicalExpression ": "off"
}
// Valid
if (this.condition) {

}

// Valid
if (this.condition &&
      this.otherConditionFunctionName() &&
        this.anotherConditionFunctionName()) {

}

// Valid
var isAConditionalVariable = this.condition &&
                                  this.otherConditionFunctionName();

What does the rule currently do for this code?

It does not check these expressions, and adding LogicalExpression to the rule config object raises errors about additional properties.

What will the rule do after it's changed?

Check additional AST nodes for indentation.

Edit: Filled out the rule change template - Thanks for linking that; apologies for not seeing it.

@eslintbot eslintbot added the triage An ESLint team member will look at this issue soon label Jul 21, 2017
@ashclarke ashclarke changed the title Indent Rule - Possible to add conditional expression? Indent Rule - Possible to add ConditionalExpression for multi-line conditions? Jul 21, 2017
@not-an-aardvark
Copy link
Member

not-an-aardvark commented Jul 21, 2017

Thanks for the proposal, but I'm confused. Isn't your example already valid with the indent rule? Filling out the rule change proposal template would be useful.

Also, I think the term you're looking for is BinaryExpression. ConditionalExpression is something else (it refers to expressions of the form a ? b : c).

@ashclarke
Copy link
Author

ashclarke commented Jul 21, 2017

Ah, forgive me. I'm still an AST node newbie.

LogicalExpression isn't in the indent rule docs either, however.

I've renamed the issue.

Edit: I've just noticed a comment in the section for LogicalExpression, here, so that might be a blocker...

/*
 * For backwards compatibility, don't check BinaryExpression indents, e.g.
 * var foo = bar &&
 *                   baz;
 */

@ashclarke ashclarke changed the title Indent Rule - Possible to add ConditionalExpression for multi-line conditions? Indent Rule - Possible to add BinaryExpression for multi-line conditions? Jul 21, 2017
@ashclarke
Copy link
Author

ashclarke commented Jul 21, 2017

Just noticed your mention about the template. I've updated the issue. Thanks!

(And sorry for not noticing it was available!)

I've changed everything to say LogicalExpression, as I believe that is the one I'm after, as per the estree node spec.

@ashclarke ashclarke changed the title Indent Rule - Possible to add BinaryExpression for multi-line conditions? Indent Rule - Possible to add LogicalExpression for multi-line conditions? Jul 21, 2017
@not-an-aardvark not-an-aardvark added enhancement This change enhances an existing feature of ESLint evaluating The team will evaluate this issue to decide whether it meets the criteria for inclusion rule Relates to ESLint's core rules and removed triage An ESLint team member will look at this issue soon labels Nov 3, 2017
@not-an-aardvark
Copy link
Member

Sorry about the delayed response here. This is something I've been working on separately from this issue -- I think it's a good idea, but we will need to make sure we can handle all of the cases in a reasonable way.

@ashclarke
Copy link
Author

No worries! I look forward to any news 👍🏻

@nzakas
Copy link
Member

nzakas commented Oct 19, 2018

Unfortunately, it looks like there wasn't enough interest from the team or community to implement this change. While we wish we'd be able to accommodate everyone's requests, we do need to prioritize. We've found that issues failing to be implemented after 90 days tend to never be implemented, and as such, we close those issues. This doesn't mean the idea isn't interesting or useful, just that it's not something the team can commit to.

@nzakas nzakas closed this as completed Oct 19, 2018
@eslint-deprecated eslint-deprecated bot locked and limited conversation to collaborators Apr 18, 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 Apr 18, 2019
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
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 evaluating The team will evaluate this issue to decide whether it meets the criteria for inclusion indent Relates to the `indent` rule rule Relates to ESLint's core rules
Projects
None yet
Development

No branches or pull requests

4 participants