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

feat: add hover display for trait assoc items #15938

Merged
merged 5 commits into from Mar 5, 2024
Merged

feat: add hover display for trait assoc items #15938

merged 5 commits into from Mar 5, 2024

Conversation

Young-Flash
Copy link
Member

This PR enable preview assoc items when hover on trait

image

inspired by #15847

@rustbot rustbot added the S-waiting-on-review Status: Awaiting review from the assignee but also interested parties. label Nov 20, 2023
@flodiebold
Copy link
Member

How does this look when hovering Iterator?

@lnicola
Copy link
Member

lnicola commented Nov 20, 2023

image

image

@lnicola
Copy link
Member

lnicola commented Nov 20, 2023

Looks like we don't display Where T: Tr, properly, and we should probably respect #[unstable] and #[doc(hidden)].

@flodiebold
Copy link
Member

IMO hiding the documentation below this is not worth it, at least not by default. Or we should at least limit it (like rustdoc collapses the long list of methods).

@Veykril
Copy link
Member

Veykril commented Nov 20, 2023

Agreed, if we do this we should at least have a config for limiting the number of items / disabling it fully, similarly how we should still do such a config for struct fields hover -> #15847 (comment)

@Young-Flash
Copy link
Member Author

Young-Flash commented Nov 20, 2023 •
edited

How does this look when hovering Iterator?

kind of ugly haha

I may not want to browse the detail of it within a hover len

@Young-Flash Young-Flash marked this pull request as draft November 24, 2023 08:04
@Veykril Veykril 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 Jan 3, 2024
@bors
Copy link
Collaborator

bors commented Jan 6, 2024

☔ The latest upstream changes (presumably #16279) made this pull request unmergeable. Please resolve the merge conflicts.

@Veykril
Copy link
Member

Veykril commented Feb 12, 2024

@Young-Flash what is the status on this PR?

@Young-Flash
Copy link
Member Author

will continue working on this after my vacation

@Young-Flash Young-Flash changed the title feat: add hover display for trait assoc items WIP: feat: add hover display for trait assoc items Feb 18, 2024
@bors
Copy link
Collaborator

bors commented Feb 20, 2024

☔ The latest upstream changes (presumably #16606) made this pull request unmergeable. Please resolve the merge conflicts.

@Young-Flash Young-Flash marked this pull request as ready for review February 22, 2024 10:07
@Young-Flash Young-Flash changed the title WIP: feat: add hover display for trait assoc items feat: add hover display for trait assoc items Feb 22, 2024
@Young-Flash
Copy link
Member Author

add "rust-analyzer.traitItemDisplayNum" (default 7) config item, now it looks like the following

image

@Young-Flash
Copy link
Member Author

will do the same for hover struct and enum if it's ok with you

@Veykril
Copy link
Member

Veykril commented Feb 27, 2024

Yes let's have a config for those as well

@Young-Flash
Copy link
Member Author

Yes let's have a config for those as well

will make it at next PR

@Young-Flash
Copy link
Member Author

how about merge this for now (anything else I can do here?)

Veykril
Veykril reviewed Feb 29, 2024
View reviewed changes
crates/rust-analyzer/src/config.rs Outdated Show resolved Hide resolved
crates/ide-db/src/defs.rs Outdated Show resolved Hide resolved
crates/hir/src/display.rs Outdated Show resolved Hide resolved
@Young-Flash
Copy link
Member Author

@rustbot ready

@rustbot rustbot added S-waiting-on-review Status: Awaiting review from the assignee but also interested parties. and removed S-waiting-on-author Status: This is awaiting some action (such as code changes or more information) from the author. labels Mar 4, 2024
@Veykril
Copy link
Member

Veykril commented Mar 5, 2024

Thanks! (will do a minor adjustment as a follow up)
@bors r+

@bors
Copy link
Collaborator

bors commented Mar 5, 2024

📌 Commit dba67b4 has been approved by Veykril

It is now in the queue for this repository.

@bors
Copy link
Collaborator

bors commented Mar 5, 2024

⌛ Testing commit dba67b4 with merge ce3216e...

@bors
Copy link
Collaborator

bors commented Mar 5, 2024

☀️ Test successful - checks-actions
Approved by: Veykril
Pushing ce3216e to master...

@bors bors merged commit ce3216e into rust-lang:master Mar 5, 2024
11 checks passed
@Young-Flash Young-Flash deleted the display_trait_item_when_hover branch March 5, 2024 08:39
@Veykril Veykril mentioned this pull request Mar 5, 2024
Merged
bors added a commit that referenced this pull request Mar 5, 2024
@bors
Auto merge of #16756 - Veykril:hover-trait, r=Veykril
124d5ff 
internal: Adjust a few things for trait assoc item hovers

#15938 (minor turned major wrt diff because of test changes)
@Young-Flash Young-Flash mentioned this pull request Mar 21, 2024
Merged
bors added a commit that referenced this pull request Mar 25, 2024
@bors
Auto merge of #16906 - Young-Flash:limit_struct_hover_display, r=Veykril
e52bb8c 
feat: limit struct hover display nums

follow up #15847, #15938
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@Veykril Veykril Veykril left review comments

Assignees
No one assigned
Labels
S-waiting-on-review Status: Awaiting review from the assignee but also interested parties.
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.

None yet
6 participants
@Young-Flash @flodiebold @lnicola @Veykril @bors @rustbot