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

multiline-ternary doesn't check parentheses #13195

Closed
mdjermanovic opened this issue Apr 18, 2020 · 7 comments · Fixed by #13367
Closed

multiline-ternary doesn't check parentheses #13195

mdjermanovic opened this issue Apr 18, 2020 · 7 comments · Fixed by #13367
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 bug ESLint is working incorrectly rule Relates to ESLint's core rules

Comments

@mdjermanovic
Copy link
Member

Tell us about your environment

  • ESLint Version: v7.0.0-alpha.3
  • Node Version: v12.14.0
  • npm Version: v6.13.4

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

Please show your full configuration:

Configuration
module.exports = {
    parserOptions: {
        ecmaVersion: 2015
    }
}

What did you do? Please include the actual source code causing the issue, as well as the command that you used to run ESLint.

Online Demo

/*eslint multiline-ternary: ["error", "always"] */

(a
) ? b
  : c;
eslint index.js

What did you expect to happen?

It might be expected that parentheses should be treated as a part of the node, so in this case the rule should report that there's a missing newline between test and consequent.

Some rules work that way (e.g., array-element-newline), some don't (function-call-argument-newline), so I'm not sure if this is a bug.

What actually happened? Please include the actual, raw output from ESLint.

no errors

Are you willing to submit a pull request to fix this bug?

yes

The example was with "always", but the same issue exists with "always-multiline" and "never".

@mdjermanovic mdjermanovic added bug ESLint is working incorrectly rule Relates to ESLint's core rules evaluating The team will evaluate this issue to decide whether it meets the criteria for inclusion labels Apr 18, 2020
@anikethsaha
Copy link
Member

I think this should be an error as well, right !

(a) 
  ? (b
    ): c; // <== error

@mdjermanovic
Copy link
Member Author

Yes, if this is a bug the same logic would apply to the check between consequent and alternate..

@eslint-deprecated eslint-deprecated bot added the auto closed The bot closed this issue label May 20, 2020
@eslint-deprecated
Copy link

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 reach accepted status after 21 days tend to
never be accepted, 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.

Thanks for contributing to ESLint and we appreciate your understanding.

@kaicataldo kaicataldo added accepted There is consensus among the team that this change meets the criteria for inclusion and removed auto closed The bot closed this issue evaluating The team will evaluate this issue to decide whether it meets the criteria for inclusion labels May 20, 2020
@kaicataldo
Copy link
Member

I personally think this should be classified as a bug, but agree that we should be consistent between rules.

@mdjermanovic
Copy link
Member Author

I'm working on this.

@jsphstls
Copy link

This change is causing some problems.

Here's the first problem from above:

(a
) ? b
  : c;

Here's what this rule allows after the MR. The ternary has not really become more readable.:

(a
)
  ? b
  : c;

This should be the only solution:

(a)
  ? b
  : c;

Another mentioned example:

(a) 
  ? (b
    ): c; // <== error

Fix allowed by the rule change:

(a) 
  ? (b
    )
  : c;

A better fix:

(a) 
  ? (b)
  : c;

This is affecting me because I cannot upgrade my core Eslint version without adding a line to every otherwise readable ternary.

This is now invalid:

a
  ? (
    b
  ) : (
    c
  )

Here's how I must now fix each of those cases:

a
  ? (
    b
  )
  : ( // extra line here
    c
  )

So why have the multiline parens? Because it handles multiline consequents while keeping alignment. This used to be allowed:

isSomething
  ? (
    <SomeJsx
      someAttr
      someOtherAttr
    />
  ) : (
    <SomeOtherJsx
      someAttr
      someOtherAttr
    />
  )

Now I am required to do this:

isSomething
  ? (
    <SomeJsx
      someAttr
      someOtherAttr
    />
  )
  : ( // extra line added here
    <SomeOtherJsx
      someAttr
      someOtherAttr
    />
  )

@mdjermanovic
Copy link
Member Author

@jsphstls thanks for the suggestion and the detailed examples!

Here's the first problem from above:

(a
) ? b
  : c;

Here's what this rule allows after the MR. The ternary has not really become more readable.:

(a
)
  ? b
  : c;

This should be the only solution:

(a)
  ? b
  : c;

I think this is correct behavior as far as this rule is concerned.

This rule doesn't control newlines in parentheses and doesn't control newlines inside the consequent/alternate expressions, so both the second and the third example are valid code for this rule, because ) (as the end of the consequent) and b are on separate lines.

We didn't change this behavior, both examples were already valid in the previous version (ESLint v7.1.0).

Another mentioned example:

(a) 
  ? (b
    ): c; // <== error

Fix allowed by the rule change:

(a) 
  ? (b
    )
  : c;

A better fix:

(a) 
  ? (b)
  : c;

Same as above. Both fixes should be valid for this particular rule, and they used to be valid before this change.

This is affecting me because I cannot upgrade my core Eslint version without adding a line to every otherwise readable ternary.

I hope we'll be able to add autofixing to this rule soon, there's ongoing PR #12982.

This is now invalid:

a
  ? (
    b
  ) : (
    c
  )

Here's how I must now fix each of those cases:

a
  ? (
    b
  )
  : ( // extra line here
    c
  )

So why have the multiline parens? Because it handles multiline consequents while keeping alignment. This used to be allowed:

isSomething
  ? (
    <SomeJsx
      someAttr
      someOtherAttr
    />
  ) : (
    <SomeOtherJsx
      someAttr
      someOtherAttr
    />
  )

Now I am required to do this:

isSomething
  ? (
    <SomeJsx
      someAttr
      someOtherAttr
    />
  )
  : ( // extra line added here
    <SomeOtherJsx
      someAttr
      someOtherAttr
    />
  )

This is a matter of preference, of course, but I'd argue that consequent and alternate seem to be more consistently aligned in the fixes than in the original versions: ? is aligned with :, and the opening parentheses are also aligned.

@eslint-deprecated eslint-deprecated bot locked and limited conversation to collaborators Dec 3, 2020
@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 Dec 3, 2020
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 bug ESLint is working incorrectly rule Relates to ESLint's core rules
Projects
None yet
Development

Successfully merging a pull request may close this issue.

4 participants