sheerun / prettier-standard

Formats with Prettier and lints with ESLint+Standard! (✿◠‿◠)
MIT License
868 stars 44 forks source link

Weird formatting of long nested ternary expression #76

Open JStonevalley opened 5 years ago

JStonevalley commented 5 years ago

Hi prettier standard formats my ternary expression without indent when breaking a line that is too long. This does not seem correct. The simplified code snippet looks as follows:

expected

When formatted it looks like this:

actual

My only config is: { "printWidth": 120 } Regards

sheerun commented 5 years ago

This would be solved after https://github.com/brodybits/prettierx/issues/44 is fixed

brodycj commented 5 years ago

brodybits/prettierx#44

I hope to resolve it soon, no promises right now.

Astra-RX commented 5 years ago

Any updates? still broken.

sheerun commented 5 years ago

I think @brodybits removed this issue from prettierx but it still needs to be fixed there. If anyone would like to see it fixed, please controbute to prettierx repository

brodycj commented 5 years ago

The issue was moved to brodybits/prettierx#13 (original repository was renamed, issue was transferred, but link did not seem to redirect). And I think the issue was already fixed by @aMarCruz.

@sheerun, I think this repo needs to use a newer version of prettierx.

sheerun commented 5 years ago

@brodybits Good to know, I'll try to upgrade this repo

aMarCruz commented 5 years ago

@sheerun , use the "alignTernaryLines": false in your prettierrc.json

with alignTernaryLines as false you have this output:

image

The logic is the same as ESLint indent.flatTernaryExpressions

sheerun commented 5 years ago

Ternary expressions should be fine in 10.0.0

sheerun commented 5 years ago

Hmm I need to revert because there are unexpected issues with other syntax

sheerun commented 5 years ago

Here's an example how prettier-standard formats right now:

const Component = () => (
  <ul className='has-text-centered'>
    {landmarks.hits.length > 0
      ? landmarks.hits.map(({ name, objectID }) => (
          <li key={name}>
            <a href={`${process.env.DOMAIN}` + `/city/${objectID}`}>{name}</a>
          </li>
        ))
      : null}
  </ul>
)

And here's how after fixing ternary expressions:

const Component = () => (
  <ul className='has-text-centered'>
    {landmarks.hits.length > 0
      ? landmarks.hits.map(({ name, objectID }) => (
        <li key={name}>
          <a href={`${process.env.DOMAIN}` + `/city/${objectID}`}>{name}</a>
        </li>
      ))
      : null}
  </ul>
)

As you can see there's missing indent even though there's no second ternary expression.

Astra-RX commented 5 years ago

lol

Astra-RX commented 5 years ago

And here's how after fixing ternary expressions:

const Component = () => (
  <ul className='has-text-centered'>
    {landmarks.hits.length > 0
      ? landmarks.hits.map(({ name, objectID }) => (
        <li key={name}>
          <a href={`${process.env.DOMAIN}` + `/city/${objectID}`}>{name}</a>
        </li>
      ))
      : null}
  </ul>
)

@sheerun It's the same result as current version of standard --fix, so let's take this step and feedback the upstream I guess?

aMarCruz commented 5 years ago

Should be. I based my PR on the ESLint rule code, which is the one that Standard uses.

sheerun commented 5 years ago

I'm sorry that this issue is this deep, but I think it should be fixed in standard first. Nested ternary expressions are far less common than single ternary with JSX code or map (as shown above) and I think this change would make formatting less readable. Formatting my project which this change re-formats tenths of files where nested ternary is not used.

sheerun commented 5 years ago

Here is another example of inconsistent formatting:

standard --fix

const Component = () => (
  <ul className='has-text-centered'>
    {landmarks.hits.length > 0
      ? landmarks.hits.map(({ name, objectID }) => {
        return someverylongvariablelololololololololololololololol
      }).map(({ name, objectID }) => {
        return <foo />
      })
      : null}
  </ul>
)

But similar code with standard --fix (with new line after before .map) yields:

const Component = () => (
  <ul className='has-text-centered'>
    {landmarks.hits.length > 0
      ? landmarks.hits
        .map(({ name, objectID }) => {
          return someverylongvariablelololololololololololololololol
        }).map(({ name, objectID }) => {
          return <foo />
        })
      : null}
  </ul>
)

On the other hand currently prettier-standard formats both examples consistently (it doesn't matter where .map is):

const Component = () => (
  <ul className='has-text-centered'>
    {landmarks.hits.length > 0
      ? landmarks.hits
          .map(({ name, objectID }) => {
            return someverylongvariablelololololololololololololololol
          })
          .map(({ name, objectID }) => {
            return <foo />
          })
      : null}
  </ul>
)
sheerun commented 5 years ago

Also, here's original example formatted with standard --fix if .map is on new line:

const Component = () => (
  <ul className='has-text-centered'>
    {landmarks.hits.length > 0
      ? landmarks.hits
        .map(({ name, objectID }) => (
          <li key={name}>
            <a href={`${process.env.DOMAIN}` + `/city/${objectID}`}>{name}</a>
          </li>
        ))
      : null}
  </ul>
)
bsiddiqui commented 4 years ago

@sheerun any update on this?

sheerun commented 4 years ago

No, someone needs to fix this at prettierx

brodycj commented 4 years ago

We already made a fix before, see brodybits/prettierx#13.

In case of troubles, someone please raise a new issue in https://github.com/brodybits/prettierx/issues with a minimal, reproducible example ([1]).

[1] https://stackoverflow.com/help/minimal-reproducible-example

sheerun commented 4 years ago

I've described issue in more detail in https://github.com/brodybits/prettierx/issues/41

So in short probably standard.js would need to be convinced first to make this change...

sheerun commented 4 years ago

So now the plan is as follows...

  1. First eslint needs to merge PR I've prepared that introduces option for indent rule that fixes this issue: https://github.com/eslint/eslint/pull/12556

  2. Then I need to convince standard to use this rule as default: https://github.com/standard/standard/issues/927

  3. Finally I need to convince prettierx to change how --no-align-ternary-line works: https://github.com/brodybits/prettierx/issues/41