return RewriteResult for rewrite_block and rewrite_closure
#6235
Conversation
ding-young
changed the title
rewrite_block and rewrite_closure
| let result = rewrite_block_with_visitor(context, &prefix, block, attrs, label, shape, true); | ||
| if let Some(ref result_str) = result { | ||
| if allow_single_line && result_str.lines().count() <= 3 { | ||
| if let rw @ Some(_) = | ||
| rewrite_single_line_block(context, &prefix, block, attrs, label, shape) | ||
| { | ||
| return rw; | ||
| } | ||
| let result_str = | ||
| rewrite_block_with_visitor(context, &prefix, block, attrs, label, shape, true)?; | ||
| if allow_single_line && result_str.lines().count() <= 3 { | ||
| if let rw @ Ok(_) = rewrite_single_line_block(context, &prefix, block, attrs, label, shape) | ||
| { | ||
| return rw; | ||
| } | ||
| } | ||
|
|
||
| result | ||
| Ok(result_str) |
There was a problem hiding this comment.
This doesn't look like a one-to-one translations from Option<String> -> RewriteResult. Also, doesn't rewrite_block_with_visitor now return RewriteResult? I would have expected to see something closer this:
let result = rewrite_block_with_visitor(context, &prefix, block, attrs, label, shape, true);
if let Ok(ref result_str) = result {
if allow_single_line && result_str.lines().count() <= 3 {
if let rw @ Ok(_) = rewrite_single_line_block(context, &prefix, block, attrs, label, shape)
{
return rw;
}
}
}There was a problem hiding this comment.
Yes, it isn't a one-to-one translation but I made this change intentionally.
From what I understood of the original code:
(1) If rewrite_block_with_visitor fails, return None.
(2) If rewrite_block_with_visitor succeeds, check whether we should try to format it into a single-line block.
a. If rewrite_single_line_block succeeds, return that result.
b. If rewrite_single_line_block fails, just return the result from rewrite_block_with_visitor.
My intention was to simplify if let Ok(ref result_str) = result {...} with using ? operator on calling rewrite_block_with_visitor. I thought it won't make a difference in control flow or formatting logic described above.
Still, if you think this change does not worth it or it actually breaks the original control flow, please tell me.
There was a problem hiding this comment.
After taking another look at this the change seems to preserve the original behavior so we're good here!
ytmimi
added
GSoC
Tracked by #6206
Description
modify the signature of
rewrite_blockandrewrite_closureto returnRewriteResultSide notes
rewrite_empty_block(it returns None when given block is not empty) since every caller triesrewrite_empty_blockfirst and then applies formatting logic for non empty block if it returns None. It seems that there's no need to propagate RewriteError upward from this function currently.