IntroduceLocal on simple names#25856
Conversation
| { | ||
| void M(int a) | ||
| { | ||
| System.Console.Write([|a|]); |
There was a problem hiding this comment.
Does thsi mean on every local/parameter/variable, i'll always get an optin to make a local out of it? #Resolved
| if (this.Expression is TTypeSyntax) | ||
| if (isSpanEmpty && this.Expression is TNameSyntax) | ||
| { | ||
| // to extract a name, you must have a selection (this avoids making the refactoring too noisy) |
There was a problem hiding this comment.
ah, nice. that seems like a good thing to do.
| return false; | ||
| } | ||
|
|
||
| if (this.Expression is TTypeSyntax && !(this.Expression is TNameSyntax)) |
There was a problem hiding this comment.
probably good to comment this.
There was a problem hiding this comment.
so condition that can hit here is
1, span not empty and expression is not name syntax
2, span is empty but expression is not name syntax
3. span not empty but expression is name syntax
so, if expression is not name syntax, regardless of span, if expression is typesyntax, we don't support refactoring.
shouldn't this check move before isSpanEmpty check?
| where TTypeSyntax : TExpressionSyntax | ||
| where TTypeDeclarationSyntax : SyntaxNode | ||
| where TQueryExpressionSyntax : TExpressionSyntax | ||
| where TNameSyntax : SyntaxNode |
There was a problem hiding this comment.
Note: TNameSyntax should probably derive from TTypeSyntax
|
@dotnet/roslyn-ide for review for 15.7. Thanks |
| { | ||
| void M(int a) | ||
| { | ||
| int {|Rename:a1|} = a; |
There was a problem hiding this comment.
ignore this. I see this use existing introduce var code. which should take care of code style.
| } | ||
|
|
||
| if (this.Expression is TTypeSyntax) | ||
| if (isSpanEmpty && this.Expression is TNameSyntax) |
There was a problem hiding this comment.
so, is it okay for the span to intersect with span of the expression? or it must be exactly the span of the expression?
There was a problem hiding this comment.
When the expression is found for a given span, there is a special case for empty span. That's why it needs to be treated specially here.
If you place your cursor on a name, the expression will be found, and it must be rejected (should not introduce variable).
If you select a portion of a name, the expression is not found.
If you select a whole name, the expression is found and it is accepted (can introduce variable).
There are tests to illustrate those scenarios (_EmptySelection, _SmallSelection).
There was a problem hiding this comment.
I see. it relies on the fact that FindNode will not return expression if selection doesn't include whole expression.
yep, I saw the test. and wondered where that rejection happens.
thank you.
|
@jinujoseph for ask-mode approval for 15.7. Or let me know if I should push out to 15.8. Thanks |
|
@Pilchie for 15.7.preview4 ask mode approval |
|
Approved for 15.7 P4. |
|
This PR makes me so happy. It's always been a frustration of mine that I can't use Introduce Local in more cases. |
Customer scenario
Allow introducing a local for simple expressions.
Bugs this fixes
Fixes #10123
Risk
Performance impact
Is this a regression from a previous update?
No