-
Notifications
You must be signed in to change notification settings - Fork 199
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
Fixed multiline FullyQualifiedNamespace Edits #1751
Changes from all commits
dbcca58
f8f4ba0
52718a4
9a18b76
c543648
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
Original file line number | Diff line number | Diff line change |
---|---|---|
|
@@ -13,13 +13,44 @@ export class RazorFullyQualifiedCodeActionTranslator implements IRazorCodeAction | |
public applyEdit( | ||
uri: vscode.Uri, | ||
edit: vscode.TextEdit): [vscode.Uri | undefined, vscode.TextEdit | undefined] { | ||
// The edit for this should just translate without additional help. | ||
throw new Error('Method not implemented.'); | ||
// 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. | ||
|
||
const newText = edit.newText.trim(); | ||
|
||
// 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. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Why this ( There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Let
In this case the
In this case the
In this case we don't need to change the |
||
const end = new vscode.Position(edit.range.start.line, edit.range.start.character + newText.length); | ||
edit.range = new vscode.Range(edit.range.start, end); | ||
} | ||
|
||
const newEdit = new vscode.TextEdit(edit.range, newText); | ||
return [ uri, newEdit ]; | ||
} | ||
|
||
public canHandleEdit(uri: vscode.Uri, edit: vscode.TextEdit): boolean { | ||
// The edit for this should just translate without additional help. | ||
return false; | ||
public canHandleEdit(uri: vscode.Uri, edit: vscode.TextEdit) { | ||
// CodeActions do not have a distinct identifier, so we must determine | ||
// if a potential edit is a Fully Qualified Namespace edit. We do so by | ||
// examining whether the new text of the edit fits one of two potential forms. | ||
const completeFullyQualifiedRegex = new RegExp('^(\\w+\\.)+\\w+$'); // Microsoft.AspNetCore.Mvc | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Added filtering logic specifically for our Fully Qualified Namespace scenario. |
||
const partialFullyQualifiedRegex = new RegExp('^(\\w+\\.)+$'); // Microsoft.AspNetCore.Mvc. | ||
|
||
const newText = edit.newText.trim(); | ||
|
||
return (!edit.range.isSingleLine && completeFullyQualifiedRegex.test(newText)) || | ||
(edit.range.start.isEqual(edit.range.end) && partialFullyQualifiedRegex.test(newText)); | ||
} | ||
|
||
public canHandleCodeAction( | ||
|
Original file line number | Diff line number | Diff line change |
---|---|---|
|
@@ -69,12 +69,19 @@ export class RazorCSharpLanguageMiddleware implements LanguageMiddleware { | |
continue; | ||
} | ||
} else { | ||
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. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Interesting. I don't remember why we call There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. 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 |
||
|
||
const newEdit = new vscode.TextEdit(remappedResponse.range, edit.newText); | ||
this.addElementToDictionary(map, documentUri, newEdit); | ||
if (codeActionUri && codeActionEdit) { | ||
this.addElementToDictionary(map, documentUri, codeActionEdit); | ||
} else { | ||
this.logger.logVerbose( | ||
`Re-mapping text ${edit.newText} at ${edit.range} in ${uri.path} to ${remappedResponse.range} in ${documentUri.path}`); | ||
|
||
this.addElementToDictionary(map, documentUri, remappedEdit); | ||
} | ||
} | ||
|
||
} | ||
} | ||
const result = this.mapToTextEdit(map); | ||
|
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.
@ajaybhargavb with your latest formatting changes, would this now be obsolete?
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.
No. What the comment says still holds true. Only that link might need to get updated.
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.
It is now here which might change again soon 😆
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.
Thanks for the clarification! 😄