Fixed multiline FullyQualifiedNamespace Edits #1751
Conversation
TanayParikh
requested review from
rynowak,
ajaybhargavb,
NTaylorMullen and
ryanbrandenburg
src/Razor/src/Microsoft.AspNetCore.Razor.VSCode/src/RazorCSharpLanguageMiddleware.ts
Outdated
Show resolved
Hide resolved
src/Razor/src/Microsoft.AspNetCore.Razor.VSCode/src/RazorCSharpLanguageMiddleware.ts
Outdated
Show resolved
Hide resolved
| // The edit for this should just translate without additional help. | ||
| return false; | ||
| public canHandleEdit(uri: vscode.Uri, edit: vscode.TextEdit) { | ||
| const completeFullyQualifiedRegex = new RegExp('^(\\w+\\.)+\\w+$'); // Microsoft.AspNetCore.Mvc |
There was a problem hiding this comment.
Added filtering logic specifically for our Fully Qualified Namespace scenario.
| // The starting and ending range may be equal in the case when we have other items on the same line. Ex: | ||
| // Render|Tree apple | ||
| // where `|` is the cursor. We want to ensure we dont't overwrite `apple` in this case with our edit. | ||
| if (newText !== edit.newText && !edit.range.start.isEqual(edit.range.end)) { |
There was a problem hiding this comment.
Why this (!edit.range.start.isEqual(edit.range.end)) condition? When will this be false?
There was a problem hiding this comment.
Let | represent the cursor.
RenderTree|
counter++;
In this case the range.start would be the R char of RenderTree and the range.end would be the last char e of RenderTree (as the edit.newText is the complete Microsoft.AspNetCore.Components.RenderTree).
!edit.range.start.isEqual(edit.range.end) == true.
RenderTree| apple
counter++;
In this case the range.start would be the R char of RenderTree and the range.end would be also be the R char of RenderTree (as the edit.newText is only Microsoft.AspNetCore.Components.).
!edit.range.start.isEqual(edit.range.end) == false.
In this case we don't need to change the range.end as the edit is "in place". We just need to trim the leading whitespace.
| this.logger.logVerbose( | ||
| `Re-mapping text ${edit.newText} at ${edit.range} in ${uri.path} to ${remappedResponse.range} in ${documentUri.path}`); | ||
| const remappedEdit = new vscode.TextEdit(remappedResponse.range, edit.newText); | ||
| const [codeActionUri, codeActionEdit] = this.tryApplyingCodeActions(uri, remappedEdit); |
There was a problem hiding this comment.
Interesting. I don't remember why we call tryApplyCodeActions when we are unable to remap edits (in the if block above). Looks like in this case, we remap successfully and then we call tryApplyCodeActions. @ryanbrandenburg @NTaylorMullen, do you recall why this was designed this way? This all seems too complicated.
There was a problem hiding this comment.
I agree, especially since the FullyQualifiedNamespace translator seems to be the only composite code action translator in use. I kept the original logic in tact (calling tryApplyCodeActions when we are unable to remap edits), in case there were some legacy reasons / code paths which could execute it. I added this call to tryApplyCodeActions on the main path so the FullyQualifiedNamespace translator can apply the necessary edits when required.
|
Thanks. I've left a note regarding the |
| // This is kind of wrong. We're manually trimming whitespace and adjusting | ||
| // for multi-line edits that're provided by O#. Right now, we do not support multi-line edits | ||
| // (ex. refactoring code actions) however there are certain supported edits which O# is automatically | ||
| // formatting for us (ex. FullyQualifiedNamespace) into multiple lines, when it should span a single line. | ||
| // This is due to how we render our virtual cs files, with fewer levels of indentation to facilitate | ||
| // appropriate error reporting (if we had additional tabs, then the error squigly would appear offset). | ||
| // | ||
| // The ideal solution for this would do something like: | ||
| // https://github.com/dotnet/aspnetcore-tooling/blob/4c8dbd0beb073e6dcee33d96d174453bf44d938a/ | ||
| // src/Razor/src/Microsoft.AspNetCore.Razor.LanguageServer/Formatting/DefaultRazorFormattingService.cs#L264 | ||
| // however we're going to hold off on that for now as it isn't immediately necessary and we don't | ||
| // (currently) support any other kind of multi-line edits. |
There was a problem hiding this comment.
@ajaybhargavb with your latest formatting changes, would this now be obsolete?
There was a problem hiding this comment.
No. What the comment says still holds true. Only that link might need to get updated.
There was a problem hiding this comment.
It is now here which might change again soon 😆
There was a problem hiding this comment.
Thanks for the clarification! 😄
Resolves the double tab spacing when using the Fully Qualified Namespace code action within @code blocks. Further details in code comments.
Fixes dotnet/aspnetcore#19449