Skip to content
This repository has been archived by the owner on Aug 31, 2023. It is now read-only.

Formatting long conditional expressions & types #2411

Closed
Tracked by #2403
MichaReiser opened this issue Apr 13, 2022 · 12 comments
Closed
Tracked by #2403

Formatting long conditional expressions & types #2411

MichaReiser opened this issue Apr 13, 2022 · 12 comments
Assignees
Labels
A-Formatter Area: formatter I-Difficult Implementation: requires deep knowledge of the tools and the problem.

Comments

@MichaReiser
Copy link
Contributor

MichaReiser commented Apr 13, 2022

Rome's formatter doesn't break long conditional expressions across multiple lines when they exceed the line width. Playground

Input

bifornCringerMoshedPerplex.bifornCringerMoshedPerplexSawder.arrayOfNumbers ? beeeeeeeeeeeeeeeeeeeeeeeeeeeeeeeeeeeeeeeeeeeeeeeeeeeeeeeeeeeee : c;

type Example1 = bifornCringerMoshedPerplex extends Animal ? beeeeeeeeeeeeeeeeeeeeeeeeeeeeeeeeeeeeeeeeeeeeeeeeeeeeeeeeeeeee : arrayOfNumbers;

// nested
bifornCringerMoshedPerplex.bifornCringerMoshedPerplexSawder.arrayOfNumbers ? beeeeeeeeeeeeeeeeeeeeeeeeeeeeeeeeeeeeeeeeeeeeeeeeeeeeeeeeeeeee : bifornCringerMoshedPerplex.bifornCringerMoshedPerplexSawder.arrayOfNumbers ? beeeeeeeeeeeeeeeeeeeeeeeeeeeeeeeeeeeeeeeeeeeeeeeeeeeeeeeeeeeee : bifornCringerMoshedPerplex.bifornCringerMoshedPerplexSawder.arrayOfNumbers ? beeeeeeeeeeeeeeeeeeeeeeeeeeeeeeeeeeeeeeeeeeeeeeeeeeeeeeeeeeeee : "test";

bifornCringerMoshedPerplex.bifornCringerMoshedPerplexSawder.arrayOfNumbers ? 
  bifornCringerMoshedPerplex.bifornCringerMoshedPerplexSawder.arrayOfNumbers ? beeeeeeeeeeeeeeeeeeeeeeeeeeeeeeeeeeeeeeeeeeeeeeeeeeeeeeeeeeeee : bifornCringerMoshedPerplex.bifornCringerMoshedPerplexSawder.arrayOfNumbers ? beeeeeeeeeeeeeeeeeeeeeeeeeeeeeeeeeeeeeeeeeeeeeeeeeeeeeeeeeeeee : "test" 
  : "other";


type Example1 = bifornCringerMoshedPerplex extends Animal ? beeeeeeeeeeeeeeeeeeeeeeeeeeeeeeeeeeeeeeeeeeeeeeeeeeeeeeeeeeeee : 
bifornCringerMoshedPerplex extends Animal ? beeeeeeeeeeeeeeeeeeeeeeeeeeeeeeeeeeeeeeeeeeeeeeeeeeeeeeeeeeeee : 
bifornCringerMoshedPerplex extends Animal ? beeeeeeeeeeeeeeeeeeeeeeeeeeeeeeeeeeeeeeeeeeeeeeeeeeeeeeeeeeeee : arrayOfNumbers;

type Example1 = bifornCringerMoshedPerplex extends Animal ? 
bifornCringerMoshedPerplex extends Animal ? beeeeeeeeeeeeeeeeeeeeeeeeeeeeeeeeeeeeeeeeeeeeeeeeeeeeeeeeeeeee : 
bifornCringerMoshedPerplex extends Animal ? beeeeeeeeeeeeeeeeeeeeeeeeeeeeeeeeeeeeeeeeeeeeeeeeeeeeeeeeeeeee : arrayOfNumbers : test;

Prettier

bifornCringerMoshedPerplex.bifornCringerMoshedPerplexSawder.arrayOfNumbers
	? beeeeeeeeeeeeeeeeeeeeeeeeeeeeeeeeeeeeeeeeeeeeeeeeeeeeeeeeeeeee
	: c;

type Example1 = bifornCringerMoshedPerplex extends Animal
	? beeeeeeeeeeeeeeeeeeeeeeeeeeeeeeeeeeeeeeeeeeeeeeeeeeeeeeeeeeeee
	: arrayOfNumbers;

// nested
bifornCringerMoshedPerplex.bifornCringerMoshedPerplexSawder.arrayOfNumbers
	? beeeeeeeeeeeeeeeeeeeeeeeeeeeeeeeeeeeeeeeeeeeeeeeeeeeeeeeeeeeee
	: bifornCringerMoshedPerplex.bifornCringerMoshedPerplexSawder.arrayOfNumbers
	? beeeeeeeeeeeeeeeeeeeeeeeeeeeeeeeeeeeeeeeeeeeeeeeeeeeeeeeeeeeee
	: bifornCringerMoshedPerplex.bifornCringerMoshedPerplexSawder.arrayOfNumbers
	? beeeeeeeeeeeeeeeeeeeeeeeeeeeeeeeeeeeeeeeeeeeeeeeeeeeeeeeeeeeee
	: "test";

bifornCringerMoshedPerplex.bifornCringerMoshedPerplexSawder.arrayOfNumbers
	? bifornCringerMoshedPerplex.bifornCringerMoshedPerplexSawder.arrayOfNumbers
		? beeeeeeeeeeeeeeeeeeeeeeeeeeeeeeeeeeeeeeeeeeeeeeeeeeeeeeeeeeeee
		: bifornCringerMoshedPerplex.bifornCringerMoshedPerplexSawder.arrayOfNumbers
		? beeeeeeeeeeeeeeeeeeeeeeeeeeeeeeeeeeeeeeeeeeeeeeeeeeeeeeeeeeeee
		: "test"
	: "other";

type Example1 = bifornCringerMoshedPerplex extends Animal
	? beeeeeeeeeeeeeeeeeeeeeeeeeeeeeeeeeeeeeeeeeeeeeeeeeeeeeeeeeeeee
	: bifornCringerMoshedPerplex extends Animal
	? beeeeeeeeeeeeeeeeeeeeeeeeeeeeeeeeeeeeeeeeeeeeeeeeeeeeeeeeeeeee
	: bifornCringerMoshedPerplex extends Animal
	? beeeeeeeeeeeeeeeeeeeeeeeeeeeeeeeeeeeeeeeeeeeeeeeeeeeeeeeeeeeee
	: arrayOfNumbers;

type Example1 = bifornCringerMoshedPerplex extends Animal
	? bifornCringerMoshedPerplex extends Animal
		? beeeeeeeeeeeeeeeeeeeeeeeeeeeeeeeeeeeeeeeeeeeeeeeeeeeeeeeeeeeee
		: bifornCringerMoshedPerplex extends Animal
		? beeeeeeeeeeeeeeeeeeeeeeeeeeeeeeeeeeeeeeeeeeeeeeeeeeeeeeeeeeeee
		: arrayOfNumbers
	: test;

Rome

bifornCringerMoshedPerplex.bifornCringerMoshedPerplexSawder.arrayOfNumbers ? beeeeeeeeeeeeeeeeeeeeeeeeeeeeeeeeeeeeeeeeeeeeeeeeeeeeeeeeeeeee : c;

type Example1 = bifornCringerMoshedPerplex extends Animal ? beeeeeeeeeeeeeeeeeeeeeeeeeeeeeeeeeeeeeeeeeeeeeeeeeeeeeeeeeeeee : arrayOfNumbers;

// nested
bifornCringerMoshedPerplex.bifornCringerMoshedPerplexSawder.arrayOfNumbers
	? beeeeeeeeeeeeeeeeeeeeeeeeeeeeeeeeeeeeeeeeeeeeeeeeeeeeeeeeeeeee
	: bifornCringerMoshedPerplex.bifornCringerMoshedPerplexSawder.arrayOfNumbers
		? beeeeeeeeeeeeeeeeeeeeeeeeeeeeeeeeeeeeeeeeeeeeeeeeeeeeeeeeeeeee
		: bifornCringerMoshedPerplex.bifornCringerMoshedPerplexSawder.arrayOfNumbers
			? beeeeeeeeeeeeeeeeeeeeeeeeeeeeeeeeeeeeeeeeeeeeeeeeeeeeeeeeeeeee
			: "test";

bifornCringerMoshedPerplex.bifornCringerMoshedPerplexSawder.arrayOfNumbers
	? bifornCringerMoshedPerplex.bifornCringerMoshedPerplexSawder.arrayOfNumbers
		? beeeeeeeeeeeeeeeeeeeeeeeeeeeeeeeeeeeeeeeeeeeeeeeeeeeeeeeeeeeee
		: bifornCringerMoshedPerplex.bifornCringerMoshedPerplexSawder.arrayOfNumbers
			? beeeeeeeeeeeeeeeeeeeeeeeeeeeeeeeeeeeeeeeeeeeeeeeeeeeeeeeeeeeee
			: "test"
	: "other";

type Example1 = bifornCringerMoshedPerplex extends Animal
	? beeeeeeeeeeeeeeeeeeeeeeeeeeeeeeeeeeeeeeeeeeeeeeeeeeeeeeeeeeeee
	: bifornCringerMoshedPerplex extends Animal
		? beeeeeeeeeeeeeeeeeeeeeeeeeeeeeeeeeeeeeeeeeeeeeeeeeeeeeeeeeeeee
		: bifornCringerMoshedPerplex extends Animal
			? beeeeeeeeeeeeeeeeeeeeeeeeeeeeeeeeeeeeeeeeeeeeeeeeeeeeeeeeeeeee
			: arrayOfNumbers;

type Example1 = bifornCringerMoshedPerplex extends Animal
	? bifornCringerMoshedPerplex extends Animal
		? beeeeeeeeeeeeeeeeeeeeeeeeeeeeeeeeeeeeeeeeeeeeeeeeeeeeeeeeeeeee
		: bifornCringerMoshedPerplex extends Animal
			? beeeeeeeeeeeeeeeeeeeeeeeeeeeeeeeeeeeeeeeeeeeeeeeeeeeeeeeeeeeee
			: arrayOfNumbers
	: test;

Expected

Rome's formatting should match Prettier's.

If we decide to keep Rome's current formatting of nested conditional expressions then add a section to Rome's website that explains why Rome's formatting differs and why we prefer it over Prettiers. The reason must be compelling enough because it makes it more difficult to adopt Rome.

@MichaReiser MichaReiser added good first issue Good for newcomers A-Formatter Area: formatter labels Apr 13, 2022
@ematipico
Copy link
Contributor

Probably an easy fix for this might be to put soft_line_break_or_space instead of the last space_token: https://github.com/rome/tools/blob/main/crates/rome_js_formatter/src/utils/format_conditional.rs#L53-L68

@MichaReiser MichaReiser changed the title Formatting long conditional expressions Formatting long conditional expressions & types Apr 13, 2022
@MichaReiser MichaReiser removed the good first issue Good for newcomers label Apr 13, 2022
@ematipico
Copy link
Contributor

@yassere @NicholasLYang

Do you think the indentation for each sub conditional helps or not?

@yassere
Copy link
Contributor

yassere commented Apr 28, 2022

For now, I think we should almost always try to match Prettier's style. It feels pretty important for adoption to reduce migration friction. Later, we can consider adding an option that enables opinionated Rome formatting if we have enough possible enhancements that it's worth it.

@ematipico
Copy link
Contributor

Later, we can consider adding an option

Too many options already 😝

Thank you for your input!

@ematipico ematipico self-assigned this Apr 29, 2022
@ematipico
Copy link
Contributor

@yassere @NicholasLYang Could you please make some sense into this code?

You're saying that we have to match prettier, but to me it seems bugged. It doesn't have any consistent logic. One branch is indented while the other is not. To me, it seems a bug.

@yassere
Copy link
Contributor

yassere commented Apr 29, 2022

I'm seeing that snippet formatted by Prettier as:

fee
	? fee
		? eee
		: feee
		? dee
		: dee
	: feee
	? fee
	: eddeefeeeffefefe
	? fddefeffefe
	: "test";

I haven't looked at the relevant Prettier code, but here's what I think is happening:

In a typical ternary chain, each JsConditionalExpression is the alternate of the one before it. And Prettier prints those without increasing the indentation level.

But if you use a JsConditionalExpression as the consequent of the one before it, it adds a level of indentation to identify that there's another "subchain" that's nested inside the outer one.

Does that make sense?

@ematipico
Copy link
Contributor

ematipico commented Apr 29, 2022

@yassere I wanted to know why the right hand side is indented while the left hand side is not (or the other way around).

Both arms have a sequence expression. One has one indentation level, while the other doesn't. To me, it's a bug.

@leops
Copy link
Contributor

leops commented Apr 29, 2022

@ematipico It has to do with how the conditional expressions are nested into each other: if you looks closely at the tokens used at the start of each branch you'll notice that sequences of ? : ? : ? : stay at the same indentation level, but two consecutive ? ? tokens will indent one level deeper (because it creates a new conditional chain). It's a detail that's really easy to miss, and that's the reason Prettier formats it like this to make it more noticeable

@ematipico
Copy link
Contributor

And now you all understand why I added the indentation, because to me it's really difficult to understand the depth of these expressions.

@ematipico ematipico assigned yassere and unassigned ematipico Apr 29, 2022
@ematipico
Copy link
Contributor

Assigning to Yasser as he understands better then me these damn expressions 😛 Thank you Yasser!

@ematipico ematipico added the I-Difficult Implementation: requires deep knowledge of the tools and the problem. label May 5, 2022
@ematipico
Copy link
Contributor

The problem is self-contained, but it requires a good understanding of how nested conditionals folds and how the grouping is applied.

@MichaReiser
Copy link
Contributor Author

This should be fixed in the latest release.

Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
A-Formatter Area: formatter I-Difficult Implementation: requires deep knowledge of the tools and the problem.
Projects
Status: Done
Development

No branches or pull requests

4 participants