New: Add offsetTernaryExpressions option for indent rule

[sheerun]

What is the purpose of this pull request?

Adds offsetTernaryExpressions boolean indent rule that solves following issues:

• Make 'indent' rule configurable w.r.t ternary conditions #6606
• [indent] Ternary doesn't allow extended indentation #7698
• Indentation/alignment within ternary branches (consequent/alternate) standard/standard#927
• Issue with indentation of expressions in ternary expression standard/standard#1451
• Weird formatting of long nested ternary expression sheerun/prettier-standard#76
• Inconsistent formatting of nested ternary expressions brodybits/prettierx#41

Please describe what the rule should do:

Functions are normally formatted with indentation which looks nice:

commission => {
  return suggestions.hits.map(hit => something(hit))
}

but when ternary expression is used suddenly return is at the same level as argument:

search.premium
  ? commission => {
    return suggestions.hits.map(hit => something(hit))
  }
  : () => onOpen()

This is just one example of many of issues described in referenced github issues, that is: the values of ternary expression are indented at the same level as ? and : instead of having its own indent level.

Here's an example with multi-level ?:. That's how it is currently:

search.premium
  ? commission => {
    return suggestions.hits.map(hit => something(hit))
  }
  : something
    ? commission => {
      return suggestions.hits.map(hit => something(hit))
    }
    : null

And that's what many developers are expecting:

search.premium
  ? commission => {
      return suggestions.hits.map(hit => something(hit))
    }
  : something
    ? commission => {
        return suggestions.hits.map(hit => something(hit))
      }
    : null

Ternary expressions are very often used in JSX so this this issue is especially annoying. For example this is how this rule formats code currently:

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 that's what many developers are expecting:

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>
)

I get that it's not possible to make this rule a default, so I'm introducing offsetTernaryExpressions rules that allows to fix the number of issues above.

Actually this rule also makes it possible to indent ternary expressions similarly to how prettier does it. Here is multiline ternary expression as formatted by prettier:

search.premium
  ? commission => {
      return suggestions.hits.map(hit => something(hit))
    }
  : something
  ? commission => {
      return suggestions.hits.map(hit => something(hit))
    }
  : null

Please note that prettier is putting ?: at the same level as nested ?:, but this rule is very separate from properly indenting commission => { ... } expressions.

What category of rule is this? (place an "X" next to just one item)

[X] Enforces code style
[ ] Warns about a potential error
[ ] Suggests an alternate way of doing something
[ ] Other (please specify:)

Provide 2-3 code examples that this rule will warn about:

search.premium
  ? commission => {
    return suggestions.hits.map(hit => something(hit))
  }
  : () => onOpen()

const object = cond
  ? {
    foo: 'bar'
  }
  : {
    baz: 'qux'
  }

Why should this rule be included in ESLint (instead of a plugin)?

Enforcing indentation is deeply integrated into eslint. It's not efficient to do it in plugin.

What changes did you make? (Give an overview)

• Add boolean offsetTernaryExpressions rules that enforces the style described above
• Add tests and documentation for it

[jsf-clabot]

All committers have signed the CLA.

[sheerun]

I've fixed some of the examples I've described in description of this issue

[Janpot]

@sheerun

And that's what many developers are expecting:

When I look at jsx rules in standard, it already fails on:

condition
  ? <x>
    y
  </x>
  : null

with

 Expected closing tag to match indentation of opening. (react/jsx-closing-tag-location)

Trying to correct it by doing

condition
  ? <x>
      y
    </x>
  : null

fails with

Expected indentation of 4 spaces but found 6.

The only way (that I know of) to satisfy standard is by doing

condition
  ? (
    <x>
      y
    </x>
  )
  : null

(Ofcourse to solve the issue, you could also put the jsx element on a single line, or store it in an intermediate variable. But that would apply tot he original problem as well, so I left those out.)

Now, it seems to me that the case presented in this PR is quite analog to the jsx one. And it seems to me that in that case, the expectation would be to format it as:

search.premium
  ? (
    commission => {
      return suggestions.hits.map(hit => something(hit))
    }
  )
  : (
    something
      ? (
        commission => {
          return suggestions.hits.map(hit => something(hit))
        }
      )
      : null
  )

Ofcourse, that is unless the jsx behavior was unintend, or unless consistency between these two variants of ternary is not important.

[sheerun]

@Janpot It seems that this PR would fix your issue as well, because:

condition
  ? <x>
      y
    </x>
  : null

doesn't return any errors when "offsetTernaryExpressions": true is used

As for your last example this PR would format it as:

search.premium
  ? (
      commission => {
        return suggestions.hits.map(hit => something(hit))
      }
    )
  : (
      something
        ? (
            commission => {
              return suggestions.hits.map(hit => something(hit))
            }
          )
        : null
    )

but the parenthesis are not necessary, and without them it formats it more nicely:

search.premium
  ? commission => {
      return suggestions.hits.map(hit => something(hit))
    }
  : something
    ? commission => {
        return suggestions.hits.map(hit => something(hit))
      }
    : null

[Janpot]

OK, seems like "unless the jsx behavior was unintend" applies here. LGTM then 👍

[Janpot]

@sheerun How would this work with indentation !== 2 spaces? What in case of > 2 spaces? What if tabs are used?

[sheerun]

@Janpot This style is not really compatible with 4-spaces indentation, because it would introduce 6-spaces indentation or 10-spaces indentation in many places...

Here's example:

search.premium
    ? commission => { // 4 spaces
          return suggestions.hits.map(hit => something(hit)) // 10 spaces?
      } // 6 spaces?
    : something

alternative:

search.premium
    ?   commission => { // 4 spaces + 2 spaces?
            return suggestions.hits.map(hit => something(hit)) // 12 spaces
        } // 8 spaces
    :   something // what?

[josephfrazier]

Given the popularity of 2-space indentation, what if we made it so that this option only applies if you also have 2-space indentation? Either that, or just trust that people without 2-space indentation won't use this option unless they're ok with the output

[ljharb]

@josephfrazier 2-space, 4-space, and tabs are all popular enough that they all should be considered.

[sheerun]

@ljharb I'm indifferent what happens if 4-spaces and tabs are used. Can you suggest what the formatting should be that eslint is going to accept? Best by example.

[ljharb]

@sheerun
correct code:

 /*eslint indent: ["error", 4, { "offsetTernaryExpressions": true }]*/

condition
    ? () => {
            return true
        }
    : condition2
     ? () => {
            return true
        }
     : () => {
            return false
        }

 /*eslint indent: ["error", 'tab', { "offsetTernaryExpressions": true }]*/

condition
	? () => {
			return true
		}
	: condition2
	? () => {
			return true
		}
	: () => {
			return false
		}

(the tabs example will ofc look horrible at the default 8-space width, but a) that's the whole point of tabs - their width is configurable, so you're not oppressed into someone else's preferred width, and b) everyone uses a 2-space or 4-space width for their tabs anyways.

[sheerun]

@ljharb I don't get you examples in some places. They deviate very much from "basic" eslint indentation rules so fixing them your way would require a lot of code and hacks. In particular nested ?: should be indented by 4 spaces and all indentation should be multiply of 4 spaces. Could you explain each comment with question mark at the end?

 /*eslint indent: ["error", 4, { "offsetTernaryExpressions": true }]*/

condition
    ? () => {
            return true // 12 spaces
        } // 8 spaces
    : condition2
     ? () => { // 7 spaces?
            return true // again 12 spaces even though nested ternary is indented? 
        } // again 8 spaces even though ternary is nested?
     : () => { // 7 spaces?
            return false // again 12 spaces even though nested ternary is indented? 
        }  // again 8 spaces even though ternary is nested?

condition
	? () => {
			return true
		}
	: condition2
	? () => { // why ? is at the same level? 
			return true
		}
	: () => {
			return false
		}

My suggestion is to use following for 4-spaces indent that is consistent:

condition
    ? () => { // 4 spaces
            return true // 12 spaces
        } // 8 spaces
    : condition2 // 4 spaces
        ? () => { // 8 spaces
                return true // 16 spaces
            } // 12 spaces
        : () => { // 8 spaces
                return false // 16 spaces
            } // 12 spaces

As you can see this makes number of spaces everywhere a multiply of 4 and makes indentation with tabs + nested indentation for ?: possible.

[ljharb]

@sheerun the 7 spaces is a mistake :-) all i did is take your 2-space example, and attempt to double the number of spaces; and then take it again and convert each 2-space to a \t.

Indentation rules should deal in indentation "levels" - i should be able to set it to 57 spaces, and 3 indentation levels thus produces 171 spaces; or 1 space, and thus 3 spaces. The number or "tab" shouldn't impact how the rules work, just which character(s) represent "one indentation level".

[sheerun]

@ljharb According to what you're saying my last example should be correct one, correct? And here's additional correct example with tabs:

condition
	? () => { // 1 tab
			return true // 3 tabs
		} // 2 tabs
	: condition2 // 1 tab
		? () => { // 2 tabs
				return true // 4 tabs
			} // 3 tabs
		: () => { // 2 tabs
				return false // 4 tabs
			} // 3 tabs

[ljharb]

Looks right to me!

[sheerun]

@ljharb Because there are no changes needed in code to format according to these examples, I've just added two more tests that verify 4-spaces and tabs indentation. I hope this makes this PR complete.

[kaicataldo]

I support this change, but can you speak to why you think it’s better to add offsetTernaryExpressions as opposed to adding a ConditionalExpression option?

[sheerun]

I don't really care what this is called. Please suggest something you're willing to merge if offsetTernaryExpressions doesn't suit you

[kaicataldo]

I will champion this.

[platinumazure]

Generally looks good to me, thanks!

Two requested changes:

1. Could you please add one or more invalid unit tests that use the new option, to show how autofix should fix the test cases into the correct format?
2. Could you please change the commit summary to begin with "Update"? Although this is a new option, it's an enhancement to an existing rule, and we traditionally reserve the "New" tag for new rules, formatters, or completely new core features.

Thanks for contributing!

[kaicataldo]

@platinumazure @ilyavolodin Any thoughts on the naming of this option, as mentioned here?

[platinumazure]

@kaicataldo I slightly prefer the current name if the implementation is otherwise unchanged.

Some thoughts:

1. All of our other node type options in this rule use numeric values (representing offset units), rather than Boolean.
2. Also, the node type options tend to set offsets for all tokens inside the node, whereas I think this option is only affecting nodes/tokens within the consequent and alternate of the ternary (and not the ? or : operator tokens). I suppose this might be okay for most users, but I find it unintuitive myself.

[kaicataldo]

That makes sense to me. Let’s stick with this API then.

[sheerun]

@platinumazure I've applied requested changes

I've added just one invalid example but it should cover all code paths, is it ok?

[kaicataldo]

Thanks for working on this. This is looking great! Just a few small comments from me.

[sheerun]

@kaicataldo done

[kaicataldo]

Could we add just one valid test with the new option set to false? Totally fine if it’s the same test as the valid test using the default value.

[sheerun]

@kaicataldo done

[kaicataldo]

This has now been accepted.

[concertcoder]

@kaicataldo hello :) what's the release schedule for this looking like?

[kaicataldo]

@concertcoder It will be published in the release after it's approved and merged (we currently do regular monthly releases). Hopefully in the next release!

[kaicataldo]

@platinumazure Have your concerns been addressed?

[platinumazure]

Looks good to me, thanks for making the changes I requested!

I would like to apologize for letting this slip through the cracks. I've been very busy with my work and haven't been able to stay on top of open source. That's not an excuse, though, and I am sorry for the poor experience.

[sheerun]

Can you release 6.9.0 out of the schedule?

[kaicataldo]

Thanks for contributing!

@sheerun Unfortunately, I don't think we'll be able to do that. This will be released in the next release!