rustfmt icon indicating copy to clipboard operation
rustfmt copied to clipboard

Improve Documentation For `single_line_if_else_max_width` in `Configurations.md` + Add Tests

Open ytmimi opened this issue 2 years ago • 3 comments

The single_line_if_else_max_width is one of many configuration options that could use some improvements to it's documentation. It would be great to:

  • Add code snippets to highlight the functionality this option provides
  • Describe that the behavior of this option changes when version=Two is set.

Additionally, some tests for the options should be added to tests/source/configs/ and tests/target/configs/. See the Create test cases section of the Contributing guide for more details. It would be great to have test cases for both version=One and version=Two.

ytmimi avatar Aug 11 '22 03:08 ytmimi

Hi @ytmimi, I would like to work on this. Could you please add me as the assignee? I am currently in the processes of trying to understand the details of how the single_line_if_else_max_width option works by stepping through the code. I might have some questions.

truls-p avatar Aug 16 '22 17:08 truls-p

That would be great! You can actually assign yourself by commenting @rustbot claim. Check out the Mastering @rustbot Guide.

ytmimi avatar Aug 16 '22 17:08 ytmimi

@rustbot claim

truls-p avatar Aug 16 '22 19:08 truls-p

While generally speaking I'm open to adding any relevant clarifying info to the docs that better explain a config option, I think the issue description has too large a scope for what I think belongs in the config doc (also noted on the linked PR).

I don't think we should mention the version=Two behavior here under the docs for this option just because both happen to operate on the construct. I think it might be reasonable to update the docs for the version option to link to the broader issue we used semi recently to catalog all the v2 differences, but I don't think we should start down the path of documenting orthogonal behavior of one option under the section for another

calebcartwright avatar Apr 16 '23 02:04 calebcartwright

Hey, I'm new to this stuff (and by stuff I mean rustfmt and GitHubs' issues) so sorry if I'm mistaken in any way or addressing my issue in the wrong place.

The first time I was reading the documentation I thought that this configuration will format any if-else expression into a single line if it's not exceeding the specified max width, so that it would look something like this:

// When the `single_line_if_else_max_width` would be set to 100...

// This code...
let foo = if true {
    1
} else {
    2
};

if false {
    println!("Hello");
} else {
    println!("World!");
}

// Would get formatted like this
let foo = if true { 1 } else { 2 };

if false { println!("Hello"); } else { println!("World!"); }

// But actually gets formatted like this
let foo = if true { 1 } else { 2 };

if false {
    println!("Hello");
} else {
    println!("World!");
}

I also thought that this configuration option would handle cases with short if statements such as this:

// Where this code...
if true {
    Err(SomeError::Foo)
}

// Would get formatted like this
if true { Err(SomeError::Foo) }

// But actually gets formatted like this
if true {
    Err(SomeError::Foo)
}

Now I know this option applies only to let statements consisting of if-else but it's not apparent from the first read of the documentation. A better documentation on this configuration or perhaps even a change of name to single_line_let_if_else_max_width or just let_if_else_max_width would be amazing as I'm not the first one to get confused with this.

It would also be great to have a configuration option for collapsing short if statements similar to the example below

// Where this code...
if !is_foo() {
    Err(SomeError::Foo)
}
if !is_bar() {
    Err(SomeError::Bar)
}

// Would get formatted like this
if !is_foo() { Err(SomeError::Foo) }
if !is_bar() { Err(SomeError::Bar) }

I know the last part consisting of a request for such a configuration doesn't belong here, so if you could tell me where to post it I would be really grateful

Frytak avatar Oct 01 '23 19:10 Frytak

@Frytak thanks for reaching out. There's some nuance with the single_line_if_else_max_width configuration that's explained pretty well in https://github.com/rust-lang/rustfmt/pull/5509 (not merged yet).

input running rustfmt --config=single_line_if_else_max_width=100,version=Two

fn contains_statements() {
    if false {
        println!("Hello");
    } else {
        println!("World!");
    }
}

fn only_expressions() {
    if false {
        println!("Hello")
    } else {
        println!("World!")
    }
}

output

fn contains_statements() {
    if false {
        println!("Hello");
    } else {
        println!("World!");
    }
}

fn only_expressions() {
    if false { println!("Hello") } else { println!("World!") }
}

The short explanation of what's going on is that the trailing ; after println!("Hello"), makes println!("Hello"); a statement, and rustfmt won't write your if-else blocks on a single line if they contain any statements. The version=Two is needed in this case because the if-else blocks are the last expression in each function definition and the single-line logic is only implemented for the last expression when version=Two is set.

Lastly, rustfmt doesn't currently rewrite if blocks on a single line without an else block. For example, we won't single line.

if !is_foo() {
    Err(SomeError::Foo)
}

ytmimi avatar Oct 03 '23 15:10 ytmimi