-
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
Convert single-line raw strings into multi-line on pressing <enter> #75648
Convert single-line raw strings into multi-line on pressing <enter> #75648
Conversation
$$following text {0} | ||
"""; | ||
""""); | ||
} |
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.
Add tests inside an interpolation.
Add tests before a quote that is or is not part of the end delimiter.
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.
Add tests when caret is on the {
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.
Added more tests and supported the above cases ptal
edit.Insert(closingStart, newLineAndIndentation); | ||
// Add a newline at the requested position | ||
edit.Insert(position, newLineAndIndentation); | ||
// Also add a newline at the start of the text, only if there is text before the requested position |
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.
Why only then?
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.
Because the user might be trying to convert the single-line into a multi-line raw string, so they'd naturally position the cursor right after the starting delimiter to press enter. That's how I've converted mine too.
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.
But then, wouldn't they want anewline added in that case as well?
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.
The logic is to follow as shown in the tests:
In """$$text"""
if I press enter it's more likely I want to just make it a multi-line raw string than to enter a new-line before the text. If I want to add two newlines there instead of one, I'm still capable of pressing enter twice to add the second newline. The purpose of this command is to auto-format the closing delimiter to avoid the compiler error.
If I put the cursor in between text and press enter, I will definitely need to reformat my raw string literal into a multi-line one so it's absolutely necessary to insert two extra newlines, one after the opening delimiter, and one before the closing one.
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 confused though. It says it only adds the newline at the start of there is text before. But in both your examples, you want to add a newline at the start
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.
Oh I see the comment being confusing. With "before the requested position" I mean the caret position, I'll update it to be more clear.
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. I see the confusion. Thanks
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.
Clarified comment
{ | ||
switch (currentStringLiteralToken.Kind()) | ||
{ | ||
case SyntaxKind.SingleLineRawStringLiteralToken: |
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.
When do we have the multi line case?
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.
Those are leftovers, will remove
This command doesn't handle multi-line raw strings and they shouldn't be expected in these methods either
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.
Multiline cases are now supported because of the underlying generalization
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 don't know what that means. it looks like earlier up we bail out in the multi-line case.
@CyrusNajmabadi ptal |
|
||
private bool MakeEdit( | ||
ITextView textView, ITextBuffer subjectBuffer, int position, Document document, | ||
ParsedDocument parsedDocument, SyntaxToken token, SyntaxToken preferredIndentationToken, |
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.
do you need document and parsedDocument?
var insertedLines = 1; | ||
if (isEmpty) | ||
{ | ||
edit.Insert(position, newLine + newLine + indentation); |
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.
doc
else | ||
{ | ||
var newLineAndIndentation = newLine + indentation; | ||
// Add a newline at the position of the end 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.
this is just stating what hte code is doing. please state why it is doing that.
edit.Insert(openingEnd, newLineAndIndentation); | ||
} | ||
} | ||
var snapshot = edit.Apply(); |
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 snapshot = edit.Apply(); | |
var snapshot = edit.Apply(); |
return tokenStart + quotes; | ||
} | ||
index++; | ||
} |
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 all needs explanations.
""""); | ||
|
||
testState.SendReturn(handled: false); | ||
} |
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.
needs tests with multi-line literals since hte code seems to be written to handle it.
@CyrusNajmabadi ready for review again |
Closes #59347
I took the liberty to fix it myself since no progress was made on the original issue despite the assignment.
CC @CyrusNajmabadi @allisonchou