Skip to content
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

Merged
merged 5 commits into from
Apr 6, 2020
Merged
Show file tree
Hide file tree
Changes from all commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
Original file line number Diff line number Diff line change
Expand Up @@ -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.
Comment on lines +16 to +27
Copy link
Contributor Author

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?

Copy link
Contributor

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.

Copy link
Contributor

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 😆

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thanks for the clarification! 😄


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)) {
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Why this (!edit.range.start.isEqual(edit.range.end)) condition? When will this be false?

Copy link
Contributor Author

@TanayParikh TanayParikh Apr 6, 2020

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

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
Copy link
Contributor Author

Choose a reason for hiding this comment

The 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(
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -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);
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

Copy link
Contributor Author

Choose a reason for hiding this comment

The 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 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.


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);
Expand Down