Support hovering limits for adts #17021
Conversation
rustbot
added
the
S-waiting-on-review
roife
force-pushed
the
add-hover-limits-for-adts
branch
2 times, most recently
from
f405b4a
to
0ec41cd
Compare
|
How about changing the default limitation to 5? |
|
I'm wondering if the change in configuration should be documented on #4604. |
That's mainly for protocol changes (extensions), but we do have a compatibility mechanism for renamed settings. |
|
☔ The latest upstream changes (presumably #16639) made this pull request unmergeable. Please resolve the merge conflicts. |
…when hovering on Self type
roife
force-pushed
the
add-hover-limits-for-adts
branch
from
edac1b5
to
6bb8598
Compare
crates/hir/src/display.rs
Outdated
| f.write_str(",\n")?; | ||
| } | ||
| f.write_str("}")?; | ||
| fn display_fields_or_variants<T: HirDisplay>( |
There was a problem hiding this comment.
I think we should split this into two, one for fields one for variants. Reason being for an enum like
enum Enum {
Variant {},
Variant2 { field: Field },
Variant3(),
Variant4(Field),
Variant5,
}hovering the enum should yield
enum Enum {
Variant {},
Variant2 { .. }, // or the field if the limit is enabled and non-zero
Variant3(),
Variant4(,.), , // or the field if the limit is enabled and non-zero
Variant5,
}That is hovering an enum should only show its shape, not its vairants fields. To see those the person can hover the variant instead.
There was a problem hiding this comment.
Variant2 { .. }, // or the field if the limit is enabled and non-zero
Here I use a single config to limit the num of fields or variants of ADTs, should we separate it into two different ones?
There was a problem hiding this comment.
Ah wait I made a mistake, hovering enums should not render the variants fields (well I said that already but my example has incorrect comments I guess).
Splitting the two into two configs sounds reasonable nevertheless.
There was a problem hiding this comment.
So, for hovering on enums, we can display it as 🤔
enum Enum {
Variant {},
Variant2 { /* .. */ },
Variant3(),
Variant4( /* .. */ ),
Variant5,
}There was a problem hiding this comment.
Yes, basically for the variants we want to use the same format that is used for when we set the config to 0 for fields on structs / unions. (which fits your example here)
Veykril
added
S-waiting-on-author
roife
force-pushed
the
add-hover-limits-for-adts
branch
from
adbefb8
to
e0e28ec
Compare
|
@rustbot ready |
rustbot
added
S-waiting-on-review
| @@ -410,8 +410,20 @@ pub(super) fn definition( | |||
| Definition::Trait(trait_) => { | |||
| trait_.display_limited(db, config.max_trait_assoc_items_count).to_string() | |||
| } | |||
| Definition::Adt(Adt::Struct(struct_)) => { | |||
| struct_.display_limited(db, config.max_struct_field_count).to_string() | |||
| Definition::Adt(adt @ (Adt::Struct(_) | Adt::Union(_))) => { | |||
There was a problem hiding this comment.
We should do this branch for Definition::Variant as well
crates/rust-analyzer/src/config.rs
Outdated
| /// How many variants of an enum to display when hovering on. Show none if empty. | ||
| hover_show_enumVariants: Option<usize> = Some(5), | ||
| /// How many fields of a struct or union to display when hovering on. Show none if empty. | ||
| hover_show_structOrUnionFields: Option<usize> = Some(5), |
There was a problem hiding this comment.
| hover_show_structOrUnionFields: Option<usize> = Some(5), | |
| hover_show_fields: Option<usize> = Some(5), |
since this should also work for enum variants, so just fields should be good enough
roife
force-pushed
the
add-hover-limits-for-adts
branch
from
99c3794
to
4357698
Compare
|
@bors r+ |
|
☀️ Test successful - checks-actions |
Fix #17009
10(instead of the originalnull), which helps users discover this feature.