Skip to content

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

Merged
merged 6 commits into from
Jun 3, 2025

Conversation

BazookaMusic
Copy link
Contributor

@BazookaMusic BazookaMusic commented May 31, 2025

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

@rustbot rustbot added the S-waiting-on-review Status: Awaiting review from the assignee but also interested parties. label May 31, 2025
Comment on lines 1156 to 1165
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)
Copy link
Member

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.

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 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.

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've removed the utility method and did it via the offset

Copy link
Member

@Veykril Veykril left a comment

Choose a reason for hiding this comment

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

Thanks!

@Veykril Veykril added this pull request to the merge queue Jun 3, 2025
Merged via the queue into rust-lang:master with commit 9edac77 Jun 3, 2025
14 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
S-waiting-on-review Status: Awaiting review from the assignee but also interested parties.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants