-
Notifications
You must be signed in to change notification settings - Fork 1.8k
Enable Assist edit for tuple<->named struct for the struct and visiblity keywords #19901
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
Conversation
crates/ide-assists/src/utils.rs
Outdated
ctx.find_node_at_offset::<ast::Name>() | ||
.and_then(|name| name.syntax().parent()) | ||
.or(ctx | ||
.token_at_offset() | ||
.find(|leaf| matches!(leaf.kind(), STRUCT_KW)) | ||
.and_then(|kw| kw.parent())) | ||
.or(ctx | ||
.find_node_at_offset::<ast::Visibility>() | ||
.and_then(|visibility| visibility.syntax().parent())) | ||
.and_then(<Either<ast::Struct, ast::Variant>>::cast) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
A simpler approach would be to do ctx.find_node_at_offset::<ast::Struct>()
, then fetch the .field_list()
and check if the cursor position is to the left of the field list, that is ctx.offset() < field_list.syntax().text_range().start()
Same for ast::Variant
. Then we can also get rid of the helper method and just do this in the corresponding assists directly.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I thought about that too, but wasn't sure if we wanted to use the offsets for this. But it is much easier to read, so I'll change to that approach.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I've removed the utility method and did it via the offset
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Thanks!
I was looking for an issue to do my first contribution to this repo. In the discussion for the issue #19666 , a suggestion was made to expand when the code action for converting between named/tuple structs is triggered to include the struct and visibility keywords, so I've added these too.
Maybe the approach of targetting specific keywords is not that nice and it should be any kind of node who parent is a struct but not the inner part of a struct?
PS. I'm new to rust and this repo, so please feel free to nitpick the code