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

Fix short-if formatting #2778

Closed
wants to merge 6 commits into from

Conversation

Projects
None yet
4 participants
@csmoe
Copy link
Member

csmoe commented Jun 9, 2018

Cloese #2777

@csmoe csmoe force-pushed the csmoe:if branch from e469bb4 to b31cfdc Jun 9, 2018

@csmoe

This comment has been minimized.

Copy link
Member Author

csmoe commented Jun 10, 2018

@nrc how can I auto-update the target files formatting when making some changes in rustfmt codebase? I used to update them with cargo run --bin rustfmt xxx.rs, but when the formatting is controlled by config, this won't work.

@topecongiro
Copy link
Collaborator

topecongiro left a comment

Thanks for the PR!

@@ -708,7 +708,7 @@ impl Rewrite for ast::Stmt {
};

let shape = shape.sub_width(suffix.len())?;
format_expr(ex, ExprType::Statement, context, shape).map(|s| s + suffix)

This comment has been minimized.

@topecongiro

topecongiro Jun 11, 2018

Collaborator

I think that this is incorrect. We should only put the if expression in a single line if it is the last statement of the block.

{
for _ in 0..10 {}
}
{ for _ in 0..10 {} }

This comment has been minimized.

@topecongiro

topecongiro Jun 11, 2018

Collaborator

This should not happen, we never put a block statement in a single line.

}
}
}
{ { { {} } } }

This comment has been minimized.

@topecongiro

topecongiro Jun 11, 2018

Collaborator

So is this one.

{
"inner-block"
}
{ "inner-block" }

This comment has been minimized.

@topecongiro

topecongiro Jun 11, 2018

Collaborator

And this one too.

@nrc

This comment has been minimized.

Copy link
Member

nrc commented Jun 11, 2018

how can I auto-update the target files formatting when making some changes in rustfmt codebase?

There's no good way to do this, sorry. If all the tests have the same config, then you can just create a rustfmt.toml and put it in some surrounding directory, then run rustfmt. Otherwise you have to do things manually.

@csmoe csmoe force-pushed the csmoe:if branch 4 times, most recently from 20aec94 to c766393 Jul 2, 2018

@csmoe

This comment has been minimized.

Copy link
Member Author

csmoe commented Jul 14, 2018

@topecongiro ping for review.

@topecongiro
Copy link
Collaborator

topecongiro left a comment

Thank you for the update! Though the current implementation is not correct yet: we only want to put an if statement on a single line only if it is the last statement.

So for example,

fn f() {
    if x { foo() } else { bar() }
    if x { foo() } else { bar() }
}

should become

fn f() {
    if x { 
        foo() 
    } else { 
        bar() 
    }
    if x { foo() } else { bar() }
}
src/expr.rs Outdated
@@ -710,7 +710,13 @@ impl Rewrite for ast::Stmt {
};

let shape = shape.sub_width(suffix.len())?;
format_expr(ex, ExprType::Statement, context, shape).map(|s| s + suffix)
let expr_type =
if stmt_is_expr(&self) && context.snippet(self.span).starts_with("if ") {

This comment has been minimized.

@topecongiro

topecongiro Jul 14, 2018

Collaborator

This is a bit hacky - you can inspect the ast::ExprKind to see whether the given expression is if or not.

@csmoe csmoe force-pushed the csmoe:if branch from c766393 to d157383 Jul 15, 2018

@csmoe csmoe force-pushed the csmoe:if branch from d157383 to 820e212 Jul 15, 2018

@petrochenkov

This comment has been minimized.

Copy link
Contributor

petrochenkov commented Feb 11, 2019

@topecongiro @csmoe
Any way to move this forward / prioritize?

@topecongiro

This comment has been minimized.

Copy link
Collaborator

topecongiro commented Feb 11, 2019

@csmoe @petrochenkov My apologies for the late reply. I missed the notification of new commits being pushed to this PR.

Since this could not get merged before 1.0 release, the formatting change needs to be version gated. Also we need to fix the merge conflicts. I think in the current form getting these done are a bit tough, so I would rather fix the issue in a separate PR.

@csmoe Again, I am very sorry that I could not review the PR in a timely manner. I really appreciate your contribution to rustfmt.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
You can’t perform that action at this time.