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

Single line if's for 1 stmt block #6031

Open
wants to merge 7 commits into
base: master
Choose a base branch
from

Conversation

Sjael
Copy link

@Sjael Sjael commented Jan 19, 2024

Fixes #5939

Copy link
Contributor

@ytmimi ytmimi left a comment

Choose a reason for hiding this comment

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

@Sjael thanks for the PR. More work is needed before we could consider moving forward.

First, this new feature should be gated behind an opt-in configuration option. Because of rustfmt's strong formatting guarantees we can't make single-line if formatting the default.

Second, we'll need many more test cases to show that this is implemented as expected.

Lastly, this option will need to interact well with existing configurations. single_line_if_else_max_width immediately comes to mind, but we probably also want to check how the new configuration interacts with control_brace_style

tests/source/expr.rs Outdated Show resolved Hide resolved
src/expr.rs Outdated Show resolved Hide resolved
src/expr.rs Outdated Show resolved Hide resolved
Comment on lines +1168 to +1185
if result.contains('\n') {
return false;
}
Copy link
Contributor

Choose a reason for hiding this comment

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

Does result contain any rewritten attributes? Would the #[allow(unused)] prevent rustfmt from writing this on a single line?

fn main() {
    #[allow(unused)]
    if true {
        println!("case 1");
    }

    #[allow(unused)]
    // What about if there's a comment here?
    if true {
        println!("case 2 with comment");
    }
}

Copy link
Contributor

Choose a reason for hiding this comment

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

We also don't want to allow single lining if the block contains a comment. I think the block rewriting handles that but just a call out that we'd need a test for it.

Copy link
Author

@Sjael Sjael Jan 20, 2024

Choose a reason for hiding this comment

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

result here is the condition string, so it's saying if we haven't gone to a new line due to the condition, proceed with checking if we can go for 1 line with the block. So yes, it contains rewritten attributes if I understand you correctly.

Copy link
Contributor

Choose a reason for hiding this comment

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

I ask because I overlooked this case when adding single_line_let_else_max_width and then we needed #5902 to correct the issue. I want to make sure that we handle this up front. It would be unfortunate if leading attributes or comments prevented the guard clause from being reformatted.

tests/source/single-line-if-else.rs Outdated Show resolved Hide resolved
@Sjael
Copy link
Author

Sjael commented Jan 24, 2024

@ytmimi Test files might need more cases, and names could be changed if needed. Let me know your thoughts on it.

@Sjael Sjael requested a review from ytmimi January 25, 2024 01:07
Configurations.md Outdated Show resolved Hide resolved
src/expr.rs Show resolved Hide resolved
Configurations.md Outdated Show resolved Hide resolved
src/expr.rs Outdated Show resolved Hide resolved
@ytmimi
Copy link
Contributor

ytmimi commented Jan 25, 2024

will take another pass at this when I have some time. In the meantime take a look at the next round of comments I left.

@Sjael
Copy link
Author

Sjael commented Jan 26, 2024

Made most of the requested changes. I also moved the test file concerning the base functionality of the feature into single_line_if/mod.rs. I didn't merge the config options, but I can do that when you decide it should be merged.

@Sjael Sjael requested a review from ytmimi January 26, 2024 04:48
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Add single line if block support for single expression
3 participants