-
Notifications
You must be signed in to change notification settings - Fork 1.9k
Don't suggest extracting out 1-tuple enum variants #6257
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
Don't suggest extracting out 1-tuple enum variants #6257
Conversation
crates/assists/src/handlers/extract_struct_from_enum_variant.rs
Outdated
Show resolved
Hide resolved
6054b25 to
d6378ed
Compare
| // Grab the tuple field inner type as a string (if it exists and is alone) | ||
| // to check against structs that may have a differing name from the enum | ||
| // variant itself | ||
| let variant_inner = { | ||
| let mut fields = field_list.fields(); | ||
| let ty = fields.next().and_then(|field| field.ty()).map(|ty| ty.to_string()); | ||
|
|
||
| match fields.next() { | ||
| Some(_) => None, | ||
| None => ty, | ||
| } | ||
| }; |
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.
Hm, why do we need this check? I'd expect something much simpler, like "if number of fields == 1, do nothing"
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'm not sure if I follow -- I was under the assumption that the fix for the issue would be to check that, for single-element tuple variants, there needs to be an additional check that no types in the parent scope that also have that name, which avoids the false positive shown in the issue. The assist would be valid for extracting out >1 element tuples regardless, therefore it checks to see if there is more than one element in the tuple.
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 mean, for
enum NameClass {
ConstReference(Definition)
}The assist should not be applicable regardnless of whether Definition exists in the scope or not. This can be implemented as a purely syntactic check, I don't think we need to look at tys here.
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.
Ah okay, so it's simply not applying the assist to 1-tuple variants. I'll go ahead and rewrite the PR to do that instead :) thanks for the clarification
d6378ed to
b9790a2
Compare
|
bors r+ Thanks! |
Fixes #6241.