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

Add unwrap block assist #4156 #4207

Merged
merged 8 commits into from
May 2, 2020
Merged

Add unwrap block assist #4156 #4207

merged 8 commits into from
May 2, 2020

Conversation

bnjjj
Copy link
Contributor

@bnjjj bnjjj commented Apr 29, 2020

close issue #4156

Signed-off-by: Benjamin Coenen <5719034+bnjjj@users.noreply.github.com>
Signed-off-by: Benjamin Coenen <5719034+bnjjj@users.noreply.github.com>
@bnjjj bnjjj requested a review from kjeremy April 29, 2020 12:54
bnjjj added 2 commits May 1, 2020 16:26
Signed-off-by: Benjamin Coenen <5719034+bnjjj@users.noreply.github.com>
// }
// ```
pub(crate) fn unwrap_block(ctx: AssistCtx) -> Option<Assist> {
let res = if let Some(if_expr) = ctx.find_node_at_offset::<ast::IfExpr>() {
Copy link
Member

Choose a reason for hiding this comment

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

This will do a bunch of tree traversal. I think we should start the assist with let l_curly = ctx.find_token_at_offset(T!['{']); and than look at it's parent up to certain depth, rather than the full set of ancestors.

Copy link
Member

Choose a reason for hiding this comment

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

In generally, it doesn't make sense to worry to much about assist perf, but it's good to not do premature pessimization if possible

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I updated with your advices. But I didn't add a .take method on ancestors iterator. Because it's a little bit subjective and I have no idea which limit we can set. You probably have a better idea so if you want to add this kind of limit let me know which limit I put.

crates/ra_assists/src/handlers/unwrap_block.rs Outdated Show resolved Hide resolved
crates/ra_assists/src/handlers/unwrap_block.rs Outdated Show resolved Hide resolved
bnjjj added 3 commits May 2, 2020 12:20
Signed-off-by: Benjamin Coenen <5719034+bnjjj@users.noreply.github.com>
Signed-off-by: Benjamin Coenen <5719034+bnjjj@users.noreply.github.com>
@bnjjj bnjjj requested a review from matklad May 2, 2020 10:32
@matklad
Copy link
Member

matklad commented May 2, 2020

bors r+

@bors
Copy link
Contributor

bors bot commented May 2, 2020

@bors bors bot merged commit 89e1f97 into rust-lang:master May 2, 2020
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants