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

conditionals: add support for nested indentation #450

Open
wants to merge 13 commits into
base: main
Choose a base branch
from
60 changes: 48 additions & 12 deletions deployment/schema.json
Original file line number Diff line number Diff line change
Expand Up @@ -345,18 +345,6 @@
"description": "Maintains the line breaks as written by the programmer."
}]
},
"conditionalExpression.linePerExpression": {
"description": "Whether to force a line per expression when spanning multiple lines.",
"type": "boolean",
"default": true,
"oneOf": [{
"const": true,
"description": "Formats with each part on a new line."
}, {
"const": false,
"description": "Maintains the line breaks as written by the programmer."
}]
},
"memberExpression.linePerExpression": {
"description": "Whether to force a line per expression when spanning multiple lines.",
"type": "boolean",
Expand Down Expand Up @@ -820,6 +808,54 @@
"binaryExpression.linePerExpression": {
"$ref": "#/definitions/binaryExpression.linePerExpression"
},
"conditionalExpression.linePerExpression": {
"description": "Whether to force a line per expression when spanning multiple lines.",
"type": "boolean",
"default": true,
"oneOf": [{
"const": true,
"description": "Formats with each part on a new line."
}, {
"const": false,
"description": "Maintains the line breaks as written by the programmer."
}]
},
"conditionalType.linePerExpression": {
"description": "Whether to force a line per expression when spanning multiple lines.",
"type": "boolean",
"default": false,
"oneOf": [{
"const": true,
"description": "Formats with each part on a new line."
}, {
"const": false,
"description": "Uses the default behaviour."
}]
},
"conditionalExpression.useNestedIndentation": {
"description": "Whether to use nested indentation within conditional expressions.",
"type": "boolean",
"default": true,
"oneOf": [{
"const": true,
"description": "Uses nested indentation in the true and false expressions."
}, {
"const": false,
"description": "Don't use nested indentation."
}]
},
"conditionalType.useNestedIndentation": {
"description": "Whether to use nested indentation within conditional types.",
"type": "boolean",
"default": true,
"oneOf": [{
"const": true,
"description": "Uses nested indentation in the true and false expressions."
}, {
"const": false,
"description": "Don't use nested indentation."
}]
},
"jsx.bracketPosition": {
"$ref": "#/definitions/jsx.bracketPosition"
},
Expand Down
28 changes: 25 additions & 3 deletions src/configuration/builder.rs
Original file line number Diff line number Diff line change
Expand Up @@ -484,12 +484,20 @@ impl ConfigurationBuilder {

/// Whether to force a line per expression when spanning multiple lines.
///
/// * `true` - Formats with each part on a new line.
/// * `false` (default) - Maintains the line breaks as written by the programmer.
/// * `true` (default) - Formats with each part on a new line.
/// * `false` - Maintains the line breaks as written by the programmer.
pub fn conditional_expression_line_per_expression(&mut self, value: bool) -> &mut Self {
self.insert("conditionalExpression.linePerExpression", value.into())
}

/// Whether to force a line per expression when spanning multiple lines.
///
/// * `true` - Formats with each part on a new line.
/// * `false` (default) - Maintains the line breaks as written by the programmer.
pub fn conditional_type_line_per_expression(&mut self, value: bool) -> &mut Self {
self.insert("conditionalType.linePerExpression", value.into())
}

/// Whether to force a line per expression when spanning multiple lines.
///
/// * `true` - Formats with each part on a new line.
Expand Down Expand Up @@ -747,6 +755,16 @@ impl ConfigurationBuilder {
self.insert("whileStatement.preferHanging", value.into())
}

/* situational indentation */

pub fn conditional_expression_use_nested_indentation(&mut self, value: bool) -> &mut Self {
self.insert("conditionalExpression.useNestedIndentation", value.into())
}

pub fn conditional_type_use_nested_indentation(&mut self, value: bool) -> &mut Self {
self.insert("conditionalType.useNestedIndentation", value.into())
}

/* force single line */

pub fn export_declaration_force_single_line(&mut self, value: bool) -> &mut Self {
Expand Down Expand Up @@ -1088,6 +1106,7 @@ mod tests {
.arrow_function_use_parentheses(UseParentheses::Maintain)
.binary_expression_line_per_expression(false)
.conditional_expression_line_per_expression(true)
.conditional_type_line_per_expression(true)
.member_expression_line_per_expression(false)
.type_literal_separator_kind(SemiColonOrComma::Comma)
.type_literal_separator_kind_single_line(SemiColonOrComma::Comma)
Expand Down Expand Up @@ -1148,6 +1167,9 @@ mod tests {
.union_and_intersection_type_prefer_hanging(true)
.variable_statement_prefer_hanging(true)
.while_statement_prefer_hanging(true)
/* situational indentation */
.conditional_expression_use_nested_indentation(false)
.conditional_type_use_nested_indentation(false)
/* member spacing */
.enum_declaration_member_spacing(MemberSpacing::Maintain)
/* next control flow position */
Expand Down Expand Up @@ -1259,7 +1281,7 @@ mod tests {
.while_statement_space_around(true);

let inner_config = config.get_inner_config();
assert_eq!(inner_config.len(), 177);
assert_eq!(inner_config.len(), 180);
let diagnostics = resolve_config(inner_config, &resolve_global_config(ConfigKeyMap::new(), &Default::default()).config).diagnostics;
assert_eq!(diagnostics.len(), 0);
}
Expand Down
4 changes: 4 additions & 0 deletions src/configuration/resolve_config.rs
Original file line number Diff line number Diff line change
Expand Up @@ -96,6 +96,7 @@ pub fn resolve_config(config: ConfigKeyMap, global_config: &GlobalConfiguration)
arrow_function_use_parentheses: get_value(&mut config, "arrowFunction.useParentheses", UseParentheses::Maintain, &mut diagnostics),
binary_expression_line_per_expression: get_value(&mut config, "binaryExpression.linePerExpression", false, &mut diagnostics),
conditional_expression_line_per_expression: get_value(&mut config, "conditionalExpression.linePerExpression", true, &mut diagnostics),
conditional_type_line_per_expression: get_value(&mut config, "conditionalType.linePerExpression", false, &mut diagnostics),
jsx_quote_style: get_value(&mut config, "jsx.quoteStyle", quote_style.to_jsx_quote_style(), &mut diagnostics),
jsx_multi_line_parens: get_value(&mut config, "jsx.multiLineParens", JsxMultiLineParens::Prefer, &mut diagnostics),
jsx_force_new_lines_surrounding_content: get_value(&mut config, "jsx.forceNewLinesSurroundingContent", false, &mut diagnostics),
Expand Down Expand Up @@ -170,6 +171,9 @@ pub fn resolve_config(config: ConfigKeyMap, global_config: &GlobalConfiguration)
union_and_intersection_type_prefer_hanging: get_value(&mut config, "unionAndIntersectionType.preferHanging", prefer_hanging, &mut diagnostics),
variable_statement_prefer_hanging: get_value(&mut config, "variableStatement.preferHanging", prefer_hanging, &mut diagnostics),
while_statement_prefer_hanging: get_value(&mut config, "whileStatement.preferHanging", prefer_hanging, &mut diagnostics),
/* situational indentation */
conditional_expression_use_nested_indentation: get_value(&mut config, "conditionalExpression.useNestedIndentation", false, &mut diagnostics),
conditional_type_use_nested_indentation: get_value(&mut config, "conditionalType.useNestedIndentation", false, &mut diagnostics),
/* member spacing */
enum_declaration_member_spacing: get_value(&mut config, "enumDeclaration.memberSpacing", MemberSpacing::Maintain, &mut diagnostics),
/* next control flow position */
Expand Down
7 changes: 7 additions & 0 deletions src/configuration/types.rs
Original file line number Diff line number Diff line change
Expand Up @@ -297,6 +297,8 @@ pub struct Configuration {
pub binary_expression_line_per_expression: bool,
#[serde(rename = "conditionalExpression.linePerExpression")]
pub conditional_expression_line_per_expression: bool,
#[serde(rename = "conditionalType.linePerExpression")]
pub conditional_type_line_per_expression: bool,
#[serde(rename = "jsx.quoteStyle")]
pub jsx_quote_style: JsxQuoteStyle,
#[serde(rename = "jsx.multiLineParens")]
Expand Down Expand Up @@ -419,6 +421,11 @@ pub struct Configuration {
pub variable_statement_prefer_hanging: bool,
#[serde(rename = "whileStatement.preferHanging")]
pub while_statement_prefer_hanging: bool,
/* situational indentation */
#[serde(rename = "conditionalExpression.useNestedIndentation")]
pub conditional_expression_use_nested_indentation: bool,
#[serde(rename = "conditionalType.useNestedIndentation")]
pub conditional_type_use_nested_indentation: bool,
/* member spacing */
#[serde(rename = "enumDeclaration.memberSpacing")]
pub enum_declaration_member_spacing: MemberSpacing,
Expand Down
46 changes: 35 additions & 11 deletions src/generation/generate.rs
Original file line number Diff line number Diff line change
Expand Up @@ -2185,9 +2185,14 @@ fn gen_conditional_expr<'a>(node: &'a CondExpr, context: &mut Context<'a>) -> Pr
let colon_token = context.token_finder.get_first_operator_after(&node.cons, ":").unwrap();
let line_per_expression = context.config.conditional_expression_line_per_expression;
let has_newline_test_cons = node_helpers::get_use_new_lines_for_nodes(&node.test, &node.cons, context.program);
let has_newline_const_alt = node_helpers::get_use_new_lines_for_nodes(&node.cons, &node.alt, context.program);
Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This is just a typo fix, const -> cons

let mut force_test_cons_newline = !context.config.conditional_expression_prefer_single_line && has_newline_test_cons;
let mut force_cons_alt_newline = !context.config.conditional_expression_prefer_single_line && has_newline_const_alt;
let has_newline_cons_alt = node_helpers::get_use_new_lines_for_nodes(&node.cons, &node.alt, context.program);
let use_new_lines_for_nested_conditional = context.config.conditional_expression_use_nested_indentation && {
node.parent().kind() == NodeKind::CondExpr || node.cons.kind() == NodeKind::CondExpr || node.alt.kind() == NodeKind::CondExpr
};
let mut force_test_cons_newline =
has_newline_test_cons && (!context.config.conditional_expression_prefer_single_line || use_new_lines_for_nested_conditional);
let mut force_cons_alt_newline =
has_newline_cons_alt && (!context.config.conditional_expression_prefer_single_line || use_new_lines_for_nested_conditional);
if line_per_expression && (force_test_cons_newline || force_cons_alt_newline) {
// for line per expression, if one is true then both should be true
force_test_cons_newline = true;
Expand Down Expand Up @@ -2295,7 +2300,7 @@ fn gen_conditional_expr<'a>(node: &'a CondExpr, context: &mut Context<'a>) -> Pr
items
};

if top_most_data.is_top_most {
if top_most_data.is_top_most || use_new_lines_for_nested_conditional {
items.push_condition(conditions::indent_if_start_of_line(cons_and_alt_items));
} else {
items.push_condition(indent_if_sol_and_same_indent_as_top_most(cons_and_alt_items, top_most_data.il));
Expand Down Expand Up @@ -5251,10 +5256,18 @@ fn gen_array_type<'a>(node: &'a TsArrayType, context: &mut Context<'a>) -> Print
}

fn gen_conditional_type<'a>(node: &'a TsConditionalType, context: &mut Context<'a>) -> PrintItems {
let use_new_lines =
!context.config.conditional_type_prefer_single_line && node_helpers::get_use_new_lines_for_nodes(&node.true_type, &node.false_type, context.program);
let top_most_data = get_top_most_data(node, context);
let is_parent_conditional_type = node.parent().kind() == NodeKind::TsConditionalType;
let line_per_expression = context.config.conditional_type_line_per_expression;
let use_new_lines_for_nested_conditional = context.config.conditional_type_use_nested_indentation && {
is_parent_conditional_type || node.true_type.kind() == NodeKind::TsConditionalType || node.false_type.kind() == NodeKind::TsConditionalType
};
let prefer_single_line = context.config.conditional_type_prefer_single_line;
let has_newline_extends_true = node_helpers::get_use_new_lines_for_nodes(&node.extends_type, &node.true_type, context.program);
let has_newline_true_false = node_helpers::get_use_new_lines_for_nodes(&node.true_type, &node.false_type, context.program);
let force_newline_extends_true = has_newline_extends_true && (!prefer_single_line || use_new_lines_for_nested_conditional);
let force_newline_true_false = has_newline_true_false && (!prefer_single_line || use_new_lines_for_nested_conditional);

let mut items = PrintItems::new();
let before_false_ln = LineNumber::new("beforeFalse");
let question_token = context.token_finder.get_first_operator_after(&node.extends_type, "?").unwrap();
Expand Down Expand Up @@ -5282,10 +5295,17 @@ fn gen_conditional_type<'a>(node: &'a TsConditionalType, context: &mut Context<'
items
})));

if question_comment_items.previous_lines.is_empty() {
items.push_signal(Signal::SpaceOrNewLine);
} else {
if !question_comment_items.previous_lines.is_empty() {
items.extend(question_comment_items.previous_lines);
} else if line_per_expression {
items.push_condition(conditions::new_line_if_multiple_lines_space_or_new_line_otherwise(
top_most_data.ln,
Some(before_false_ln),
));
} else if force_newline_extends_true {
items.push_signal(Signal::NewLine);
} else {
items.push_signal(Signal::SpaceOrNewLine);
}

items.push_condition({
Expand Down Expand Up @@ -5314,7 +5334,7 @@ fn gen_conditional_type<'a>(node: &'a TsConditionalType, context: &mut Context<'
items.extend(colon_comment_items.previous_lines);

// false type
if use_new_lines {
if force_newline_true_false {
items.push_signal(Signal::NewLine);
} else {
items.push_condition(conditions::new_line_if_multiple_lines_space_or_new_line_otherwise(
Expand Down Expand Up @@ -5342,7 +5362,11 @@ fn gen_conditional_type<'a>(node: &'a TsConditionalType, context: &mut Context<'
items
};

if is_parent_conditional_type {
// Unless `use_nested_indentation` is enabled, we keep the indentation the same. e.g.:
// type A<T> = T extends string ? 'string'
// : T extends number ? 'number'
// : T extends boolean ? 'boolean';
if !use_new_lines_for_nested_conditional && is_parent_conditional_type {
items.extend(false_type_generated);
} else {
items.push_condition(conditions::indent_if_start_of_line(false_type_generated));
Expand Down
Original file line number Diff line number Diff line change
@@ -0,0 +1,38 @@
~~ lineWidth: 50, conditionalExpression.useNestedIndentation: true ~~
== should handle nested indentation on a basic ternary / conditional expression ==
const value = is_prod
? do1()
: is_laptop
? do2()
: do3();

[expect]
const value = is_prod
? do1()
: is_laptop
? do2()
: do3();

== should keep on single line if fits on a single line ==
const value = a ? b ? c : d : e;

[expect]
const value = a ? b ? c : d : e;

== should go multi-line when exceeding the line width ==
const value = testing ? this ? out : testing : exceedsWidth;

[expect]
const value = testing
? this ? out : testing
: exceedsWidth;

== should both go multi-line when exceeding the line width twice ==
const value = testing ? this ? out : testingTestingTestingTestingTesting : exceedsWidth;

[expect]
const value = testing
? this
? out
: testingTestingTestingTestingTesting
: exceedsWidth;
Original file line number Diff line number Diff line change
@@ -0,0 +1,20 @@
~~ lineWidth: 80, conditionalType.linePerExpression: true ~~
== should use a line per expression when multiLine ==
type Test = A extends B
? C extends D
? c
: testingTesting
: testingTestingTestingTesting;

[expect]
type Test = A extends B
? C extends D
? c
: testingTesting
: testingTestingTestingTesting;
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@declanvong sorry for my delay here. I just added this file and test, but it fails. I think this is the expected output?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Yeah, looks like that maybe one of the indent conditions is listening to linePerExpression instead of useNestedIndentation? I'll take a look

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Fixed, but important to note that this change is not backwards compatible, it changes existing formatting, so it'd be a version bump.
There's a test with some existing formatting that I've updated (ConditionalType_All.txt::should format an individual condition that goes over the limit).

Since both linePerExpression and useNestedIndentation are not enabled in that test, it goes from being formatted originally like this:

type Type<T> = T extends string ? number
    : T extends hereIsAVeryLongExtendsClause
        ? testingThis
    : boolean;

to this (flattened indentation):

type Type<T> = T extends string ? number
    : T extends hereIsAVeryLongExtendsClause
    ? testingThis
    : boolean;

Enabling useNestedIndentation without linePerExpression gives:

type Type<T> = T extends string ? number
    : T extends hereIsAVeryLongExtendsClause
        ? testingThis
        : boolean;

which also seems correct.

So there's no way to get the original formatting in this new world, but maybe that's good? if the original formatting was always not technically correct or wanted. after all, that trailing : boolean really is part of the inner conditional, not the outer one, so I feel like the new formatting is better anyway as it makes that more obvious.


== should keep on single line if fits on a single line ==
type Test = A extends B ? C extends D ? c : d : e;

[expect]
type Test = A extends B ? C extends D ? c : d : e;
Original file line number Diff line number Diff line change
@@ -0,0 +1,26 @@
~~ lineWidth: 80, conditionalType.useNestedIndentation: true, conditionalType.linePerExpression: true ~~
== should handle nested indentation with extends and mapped types ==
export type R<S> = S extends Record<string, T<any, any>> ? { [K in keyof S]: ReturnType<S[K][1]> } : S extends T<infer _T, any>
? _T extends Array<infer I>
? I
: _T : S extends SZ<infer _T, any> ? _T : S extends DZ<infer _T>
? _T : never;

[expect]
export type R<S> = S extends Record<string, T<any, any>>
? { [K in keyof S]: ReturnType<S[K][1]> }
: S extends T<infer _T, any>
? _T extends Array<infer I>
? I
: _T
: S extends SZ<infer _T, any>
? _T
: S extends DZ<infer _T>
? _T
: never;

== should keep on single line if fits on a single line ==
type Test = A extends B ? C extends D ? c : d : e;

[expect]
type Test = A extends B ? C extends D ? c : d : e;
Original file line number Diff line number Diff line change
@@ -0,0 +1,25 @@
~~ lineWidth: 80, conditionalType.useNestedIndentation: true ~~
== should handle nested indentation with extends and mapped types ==
export type R<S> = S extends Record<string, T<any, any>> ? { [K in keyof S]: ReturnType<S[K][1]> } : S extends T<infer _T, any>
? _T extends Array<infer I>
? I
: _T : S extends SZ<infer _T, any> ? _T : S extends DZ<infer _T>
? _T : never;

[expect]
export type R<S> = S extends Record<string, T<any, any>>
? { [K in keyof S]: ReturnType<S[K][1]> }
: S extends T<infer _T, any>
? _T extends Array<infer I>
? I
: _T
: S extends SZ<infer _T, any> ? _T
: S extends DZ<infer _T>
? _T
: never;

== should keep on single line if fits on a single line ==
type Test = A extends B ? C extends D ? c : d : e;

[expect]
type Test = A extends B ? C extends D ? c : d : e;