-
Notifications
You must be signed in to change notification settings - Fork 4k
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
Refactoring provider to convert regular string to interpolated string #48502
Conversation
...ertToInterpolatedString/CSharpConvertRegularStringToInterpolatedStringRefactoringProvider.cs
Outdated
Show resolved
Hide resolved
=> isVerbatim | ||
? text.Substring("@'".Length, text.Length - "@''".Length) | ||
: text.Substring("'".Length, text.Length - "''".Length); |
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.
This is less repetitive, and IMO still readable.
=> isVerbatim | |
? text.Substring("@'".Length, text.Length - "@''".Length) | |
: text.Substring("'".Length, text.Length - "''".Length); | |
=> text[(isVerbatim ? "@'" : "'").Length..]; |
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.
That's not the same result, you're missing ^1
on the end.
Personally I find it confusing that this uses the string length of a literal to work out how many characters to remove, particularly as the literals don't have the right quotation marks in them, nor would the text in question have double quotes. I also don't see why this can't be in the language agnostic layer, since isVerbatim
will always be false for VB anyway. I would simply write this as:
private static string GetTextWithoutQuotes(string text, bool isVerbatim)
{
// Trim off an extra character for the @ symbol for verbatim strings
var startIndex = isVerbatim ? 2 : 1;
return text[startIndex..^1];
}
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.
also, please onl yhave this refactoring if there are no diagnotics on the expr. and probably a good time to ensure that the string is terminated. and pleas have test for thie case "foo{
(i.e. an unterminated literal).
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.
That's not the same result, you're missing
^1
on the end.Personally I find it confusing that this uses the string length of a literal to work out how many characters to remove, particularly as the literals don't have the right quotation marks in them, nor would the text in question have double quotes. I also don't see why this can't be in the language agnostic layer, since
isVerbatim
will always be false for VB anyway. I would simply write this as:private static string GetTextWithoutQuotes(string text, bool isVerbatim) { // Trim off an extra character for the @ symbol for verbatim strings var startIndex = isVerbatim ? 2 : 1; return text[startIndex..^1]; }
I agree, this is a better way of doing things.
(The code I used was copied from https://github.com/dotnet/roslyn/blob/master/src/Features/CSharp/Portable/ConvertToInterpolatedString/CSharpConvertConcatenationToInterpolatedStringRefactoringProvider.cs#L25)
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.
feel free to extract all of this into a helper utilities class they can both call into.
...tToInterpolatedString/AbstractConvertRegularStringToInterpolatedStringRefactoringProvider.cs
Outdated
Show resolved
Hide resolved
...tToInterpolatedString/AbstractConvertRegularStringToInterpolatedStringRefactoringProvider.cs
Outdated
Show resolved
Hide resolved
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.
Does something like the following work?
public class C
{
public void M(string code = @"class Program{}")
{
}
}
If it works, add a test for it.
If it doesn't work, then this is an edge case that IMO can be addressed later, but it's up to the team whether they want it in this PR or to be addressed later. (I think it may not be hard to add it if it doesn't currently work)
Just noticed how crap I just wrote 😄
This may be something to check with const interpolated strings if it came with C# 10.
=> isVerbatim | ||
? text.Substring("@'".Length, text.Length - "@''".Length) | ||
: text.Substring("'".Length, text.Length - "''".Length); |
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.
That's not the same result, you're missing ^1
on the end.
Personally I find it confusing that this uses the string length of a literal to work out how many characters to remove, particularly as the literals don't have the right quotation marks in them, nor would the text in question have double quotes. I also don't see why this can't be in the language agnostic layer, since isVerbatim
will always be false for VB anyway. I would simply write this as:
private static string GetTextWithoutQuotes(string text, bool isVerbatim)
{
// Trim off an extra character for the @ symbol for verbatim strings
var startIndex = isVerbatim ? 2 : 1;
return text[startIndex..^1];
}
...tToInterpolatedString/AbstractConvertRegularStringToInterpolatedStringRefactoringProvider.cs
Outdated
Show resolved
Hide resolved
Could you show a picture of the refactoring and lightbulb? |
...ures/CSharpTest/ConvertToInterpolatedString/ConvertRegularStringToInterpolatedStringTests.cs
Show resolved
Hide resolved
...ures/CSharpTest/ConvertToInterpolatedString/ConvertRegularStringToInterpolatedStringTests.cs
Show resolved
Hide resolved
...tToInterpolatedString/AbstractConvertRegularStringToInterpolatedStringRefactoringProvider.cs
Outdated
Show resolved
Hide resolved
...tToInterpolatedString/AbstractConvertRegularStringToInterpolatedStringRefactoringProvider.cs
Outdated
Show resolved
Hide resolved
var startToken = generator.CreateInterpolatedStringStartToken(isVerbatimStringLiteral) | ||
.WithLeadingTrivia(literalExpression.GetLeadingTrivia()); | ||
var endToken = generator.CreateInterpolatedStringEndToken() | ||
.WithTrailingTrivia(literalExpression.GetTrailingTrivia()); |
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.
var startToken = generator.CreateInterpolatedStringStartToken(isVerbatimStringLiteral) | |
.WithLeadingTrivia(literalExpression.GetLeadingTrivia()); | |
var endToken = generator.CreateInterpolatedStringEndToken() | |
.WithTrailingTrivia(literalExpression.GetTrailingTrivia()); | |
var startToken = generator.CreateInterpolatedStringStartToken(isVerbatimStringLiteral).WithLeadingTrivia(literalExpression.GetLeadingTrivia()); | |
var endToken = generator.CreateInterpolatedStringEndToken().WithTrailingTrivia(literalExpression.GetTrailingTrivia()); |
...tToInterpolatedString/AbstractConvertRegularStringToInterpolatedStringRefactoringProvider.cs
Outdated
Show resolved
Hide resolved
Very close IMO. Only need some minor changes. |
...tToInterpolatedString/AbstractConvertRegularStringToInterpolatedStringRefactoringProvider.cs
Outdated
Show resolved
Hide resolved
...tToInterpolatedString/AbstractConvertRegularStringToInterpolatedStringRefactoringProvider.cs
Outdated
Show resolved
Hide resolved
...ertToInterpolatedString/CSharpConvertRegularStringToInterpolatedStringRefactoringProvider.cs
Outdated
Show resolved
Hide resolved
...tToInterpolatedString/AbstractConvertRegularStringToInterpolatedStringRefactoringProvider.cs
Outdated
Show resolved
Hide resolved
...tToInterpolatedString/AbstractConvertRegularStringToInterpolatedStringRefactoringProvider.cs
Outdated
Show resolved
Hide resolved
...tToInterpolatedString/AbstractConvertRegularStringToInterpolatedStringRefactoringProvider.cs
Outdated
Show resolved
Hide resolved
...InterpolatedString/VisualBasicConvertRegularStringToInterpolatedStringRefactoringProvider.vb
Outdated
Show resolved
Hide resolved
@CyrusNajmabadi, there are still a few unresolved conversations... Personally I feel they are not showstoppers, but LMK if you feel otherwise and I will attempt to address them. And thanks again for all your input. |
...e/ConvertToInterpolatedString/ConvertRegularStringToInterpolatedStringRefactoringProvider.cs
Outdated
Show resolved
Hide resolved
…RegularStringToInterpolatedStringRefactoringProvider.cs
...e/ConvertToInterpolatedString/ConvertRegularStringToInterpolatedStringRefactoringProvider.cs
Outdated
Show resolved
Hide resolved
…RegularStringToInterpolatedStringRefactoringProvider.cs
...e/ConvertToInterpolatedString/ConvertRegularStringToInterpolatedStringRefactoringProvider.cs
Outdated
Show resolved
Hide resolved
…RegularStringToInterpolatedStringRefactoringProvider.cs
...e/ConvertToInterpolatedString/ConvertRegularStringToInterpolatedStringRefactoringProvider.cs
Outdated
Show resolved
Hide resolved
…RegularStringToInterpolatedStringRefactoringProvider.cs
...e/ConvertToInterpolatedString/ConvertRegularStringToInterpolatedStringRefactoringProvider.cs
Outdated
Show resolved
Hide resolved
…RegularStringToInterpolatedStringRefactoringProvider.cs
...e/ConvertToInterpolatedString/ConvertRegularStringToInterpolatedStringRefactoringProvider.cs
Outdated
Show resolved
Hide resolved
…RegularStringToInterpolatedStringRefactoringProvider.cs
Hey @CyrusNajmabadi, I noticed some checks had failed. Do I need to do anything else on my end to get the PR merged? Do you need me to rebase? |
rerunning test now. LMK when you're green :) |
Still not green. :( |
Ok @CyrusNajmabadi, we're good to go. 😉 |
Awesome! |
thanks! |
Fixes #48219
The refactoring is offered for regular string literals that