Port from CSharpEssentials of Convert-To-Interpolated-String #11415
Conversation
|
@dotnet/roslyn-ide |
jmarolf
force-pushed
the
interpolated-string-refactoring
branch
2 times, most recently
from
0523c51
to
9c375ea
Compare
| { | ||
| internal partial class ConvertToInterpolatedStringRefactoringProvider | ||
| { | ||
| private class InterpolatedStringRewriter : CSharpSyntaxRewriter |
There was a problem hiding this comment.
Since you're only rewriting InterpolationSyntax, couldn't you call ReplaceNodes rather than creating a rewriter?
|
Could you describe what the custom formatting rule is used for and why it should be specific to this refactoring rather than pushing it down into the formatting engine? |
|
@DustinCampbell today the formatting ending will always place |
jmarolf
force-pushed
the
interpolated-string-refactoring
branch
from
9c375ea
to
25ebeb2
Compare
| } | ||
|
|
||
| [Fact, Trait(Traits.Feature, Traits.Features.CodeActionsConvertToInterpolatedString)] | ||
| public async Task TestItemOutsideRange() |
There was a problem hiding this comment.
Is this what we actually want? Yes the old code is broken but this seems a pretty strange way to "improve" it.
There was a problem hiding this comment.
Do we want to trigger a warning? Would you prefer us to not offer the refactoring in this case? Typically always allow a user to perform a refactoring, even if their code doesn't make sense
There was a problem hiding this comment.
I'm not following what you guys are talking about here. The test range below seems reasonable to me.
|
@jmarolf -- shouldn't we just make the elastic trivia rule smarter? Otherwise, any generate code going into an interpolation would have to do the same thing. |
|
@DustinCampbell I agree, I'll work on fixing up the formatter for interpolated strings and remove the custom formatting logic. |
jmarolf
force-pushed
the
interpolated-string-refactoring
branch
from
25ebeb2
to
63dde06
Compare
removing syntax rewriter and incorporating formatting rules into formatter
jmarolf
force-pushed
the
interpolated-string-refactoring
branch
from
63dde06
to
725c75d
Compare
|
retest windows_vsi_p2_open_prtest please |
| namespace Microsoft.CodeAnalysis.CSharp.CodeRefactorings.ConvertToInterpolatedString | ||
| { | ||
| [ExportCodeRefactoringProvider(LanguageNames.CSharp, Name = PredefinedCodeRefactoringProviderNames.ConvertToInterpolatedString), Shared] | ||
| internal partial class ConvertToInterpolatedStringRefactoringProvider : AbstractConvertToInterpolatedStringRefactoringProvider<InterpolatedStringExpressionSyntax, InvocationExpressionSyntax, ExpressionSyntax, ArgumentSyntax, LiteralExpressionSyntax> |
| formatMethods As ImmutableArray(Of ISymbol), | ||
| semanticModel As SemanticModel, | ||
| cancellationToken As CancellationToken) As Boolean | ||
| If (invocation.ArgumentList IsNot Nothing AndAlso |
There was a problem hiding this comment.
This logic also looks teh same as the C# version. Can we pull up into the base type?
jmarolf
force-pushed
the
interpolated-string-refactoring
branch
from
18e0fd5
to
6ad9b9c
Compare
| protected override InterpolatedStringExpressionSyntax GetInterpolatedString(string text) => | ||
| ParseExpression("$" + text) as InterpolatedStringExpressionSyntax; | ||
|
|
||
| protected override LiteralExpressionSyntax GetLiteralExpressionSyntax(SeparatedSyntaxList<ArgumentSyntax> arguments) => |
There was a problem hiding this comment.
How is this function different from GetFirstArgument?
jmarolf
force-pushed
the
interpolated-string-refactoring
branch
from
6ad9b9c
to
6037af5
Compare
| protected override SyntaxList<InterpolatedStringContentSyntax> GetInterpolatedStringContents(InterpolatedStringExpressionSyntax interpolatedString) => | ||
| interpolatedString.Contents; | ||
|
|
||
| protected override SyntaxNode ReplaceExpressionInInterpolation(InterpolationSyntax node, ExpressionSyntax expressionSyntax) => |
There was a problem hiding this comment.
Could be lifted up to the base type. Just call. "node.ReplaceNode(GetExpressionOfInterpolation(), expressionSyntax)"
|
LGTM. Mainly nits at ths point. |
jmarolf
force-pushed
the
interpolated-string-refactoring
branch
2 times, most recently
from
9967351
to
9b84412
Compare
| @@ -573,5 +573,15 @@ IEnumerator IEnumerable.GetEnumerator() | |||
|
|
|||
| return SpecializedCollections.EmptyEnumerator<TNode>(); | |||
| } | |||
|
|
|||
| public static implicit operator SeparatedSyntaxList<SyntaxNode>(SeparatedSyntaxList<TNode> nodes) | |||
There was a problem hiding this comment.
@dotnet/roslyn-compiler Please take a look at this. Current SyntaxList<T> has these operators, but we missed them for SeparatedSyntaxList. Anyone have a problem with us adding this?
There was a problem hiding this comment.
- general logic cleanup - moving methods to abstract base class
jmarolf
force-pushed
the
interpolated-string-refactoring
branch
from
9b84412
to
baf565f
Compare
|
test vsi please |
No description provided.