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.

Sign up for GitHub

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

Jump to bottom

limit change_visibility assist to applicable items #15277

Merged
merged 1 commit into from
Jul 22, 2023
Merged

limit change_visibility assist to applicable items #15277

merged 1 commit into from
Jul 22, 2023
+68 −7

Conversation

1Kinoti
Copy link
Contributor

@1Kinoti 1Kinoti commented Jul 13, 2023 •
edited
Loading

this pr limits the change_visibility assist to applicable items. top level items in this context means items that are not nested within fns or traits.

now

fn foo {
    // assists on this `struct` keyword won't include `change_visibility`
    struct Bar {} 
}

trait Foo {
   // same with the `fn` here
   fn bar();
}

@rustbot rustbot added the S-waiting-on-review Status: Awaiting review from the assignee but also interested parties. label Jul 13, 2023
lowr
lowr reviewed Jul 15, 2023
View reviewed changes
Copy link
Contributor

@lowr lowr left a comment

Choose a reason for hiding this comment

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

Yeah, I agree that we don't want to provide this assist for nested items. Left several suggestions.

crates/ide-assists/src/handlers/change_visibility.rs Outdated Show resolved Hide resolved
crates/ide-assists/src/handlers/change_visibility.rs Outdated Show resolved Hide resolved
crates/ide-assists/src/handlers/change_visibility.rs Outdated Show resolved Hide resolved
@lowr lowr added S-waiting-on-author Status: This is awaiting some action (such as code changes or more information) from the author. and removed S-waiting-on-review Status: Awaiting review from the assignee but also interested parties. labels Jul 16, 2023
@1Kinoti 1Kinoti changed the title limit change_visibility assist to top level items limit change_visibility assist to applicable items Jul 19, 2023
@1Kinoti 1Kinoti requested a review from lowr July 19, 2023 09:27
@lowr
Copy link
Contributor

lowr commented Jul 19, 2023

I'd like to see two more things, but otherwise looks good to me!

  • Can you add a comment in fn can_add() explaining why inherent impls are exempted?
  • We generally don't want to include merge commits. Can you remove a81498a and maybe squash all the commits into one?

@1Kinoti 1Kinoti marked this pull request as draft July 22, 2023 14:17
@1Kinoti 1Kinoti marked this pull request as ready for review July 22, 2023 14:18
@1Kinoti
Copy link
Contributor Author

1Kinoti commented Jul 22, 2023

@lowr done

@lowr
Copy link
Contributor

lowr commented Jul 22, 2023

Nice, thanks!

@bors r+

@bors
Copy link
Contributor

bors commented Jul 22, 2023

📌 Commit 65823b0 has been approved by lowr

It is now in the queue for this repository.

@bors
Copy link
Contributor

bors commented Jul 22, 2023

⌛ Testing commit 65823b0 with merge b17be27...

@bors
Copy link
Contributor

bors commented Jul 22, 2023

☀️ Test successful - checks-actions
Approved by: lowr
Pushing b17be27 to master...

@bors bors merged commit b17be27 into rust-lang:master Jul 22, 2023
@1Kinoti 1Kinoti deleted the pub-assist branch July 22, 2023 15:18
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@lowr lowr Awaiting requested review from lowr

Assignees
No one assigned
Labels
S-waiting-on-author Status: This is awaiting some action (such as code changes or more information) from the author.
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
4 participants
@1Kinoti @lowr @bors @rustbot