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.

Sign up for GitHub

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

Normalize code actions indentation #20

Closed
wants to merge 4 commits into from
Closed

Normalize code actions indentation #20

Changes from all commits
Commits
Show all changes
4 commits
Select commit Hold shift + click to select a range
af6734c
Normalize code actions indentation
zardoy Mar 16, 2023
9d9c128
patch renameLocation for all known problem cases
zardoy Mar 16, 2023
efda54d
format
zardoy Mar 16, 2023
a19f3cc
Merge branch 'master' into pr/20
johnsoncodehk Mar 21, 2023
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
29 changes: 29 additions & 0 deletions packages/vscode/src/index.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -29,6 +29,9 @@ export const middleware: lsp.Middleware = {
if (action.command) {
action.command = parseServerCommand(action.command);
}
if (action.edit) {
normalizeCodeActionEdit(document, action);
}
Comment on lines 29 to +34
Copy link
Contributor

Choose a reason for hiding this comment

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

The way servers like typescript-language-server handle that is they ensure that Configure request is always sent for the current file before getting code actions or doing other relevant requests. Is that not possible here?

Line endings is just one of the aspects. There are also other formatting choices like whether to use spaces or not, whether to use semicolons etc. So this doesn't seem like a very comprehensive solution.

Copy link
Author

Choose a reason for hiding this comment

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

@rchl I'm quite confused.

Line endings is just one of the aspects.

Since the package is intended for vscode only, we should never care about it, thats why it is always set to \n.
Other format options are already used, the only issue was formatOptions that contains indentation information

normalizeCodeActionEdit only changes indentation, I'm replicating vscode behavior: https://github.com/microsoft/vscode/blob/3059063b805ed0ac10a6d9539e213386bfcfb852/extensions/typescript-language-features/src/languageFeatures/fileConfigurationManager.ts#L59-L62

But you're right ideally it should be passed during request, but as I understood from vuejs/language-tools#2498 (comment) its currently not possible / no easy way to do it. Though, AFAICS from my use cases the current solution works ideally

Copy link
Contributor

Choose a reason for hiding this comment

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

I was indeed confused about line breaks vs. indentation.

But the same point still stands that indentation style is just one of the options passed through the Configure request so ideally that request should be made.

I don't know code base well enough to say whether it's feasible to do so from this point I'm just deferring to @johnsoncodehk to give his take and decide whether that's fine.

Copy link
Member

Choose a reason for hiding this comment

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

I hope to avoid use middleware handling it, which causes other language clients to have to follow if they want the same functionality. I think it would be best to raise this requirement to the LSP repo. Also replaceAll() may break \t in TemplateLiteral.

If this feature is important, these two approaches may work:

  1. Infer FormattingOptions from the upper and lower codes, I think there will be ready-made solutions.
  2. Write the FormattingOptions of the formatting request to tmpdir() or similar to share it.

Copy link
Author

Choose a reason for hiding this comment

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

2. Write the FormattingOptions of the formatting request to tmpdir() or similar to share it.

Isn't creating custom request such as sync format options much easier?

Though it is much easier to go with first way, so, let's infer code-style in place.

Copy link
Member

Choose a reason for hiding this comment

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

Because custom request requires downstream language clients to implement, I hope to reduce the necessary work for the language clients.

Copy link
Author

Choose a reason for hiding this comment

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

Though it is much easier to go with the first way, so, let's infer code-style in place.

Done here: volarjs/services@65c2a15 so I'm closing this pr

return action;
});
return actions;
Expand Down Expand Up @@ -84,3 +87,29 @@ export function parseServerCommand(command: vscode.Command) {
}
return command;
}

export const normalizeCodeActionEdit = (document: vscode.TextDocument, action: vscode.CodeAction) => {
if (!action.edit) return;
const editor = vscode.window.visibleTextEditors.find(editor => editor.document.fileName === document.fileName);
if (!editor) return;
const { options: { insertSpaces, tabSize } } = editor;
if (!insertSpaces) return;

const newEdit = new vscode.WorkspaceEdit();
const renamePos = action.command?.command === 'editor.action.rename' && action.command.arguments![0][1] as vscode.Position;

for (const [uri, edits] of action.edit.entries()) {
newEdit.set(uri, edits.map(edit => {
// #region patch renameLocation for extract symbol actions
if (edit.newText.startsWith('\t') && renamePos) {
const endPos = document.positionAt(document.offsetAt(edit.range.start) + edit.newText.length);
if (new vscode.Range(edit.range.start, endPos).contains(renamePos)) {
action.command!.arguments![0][1] = renamePos.translate(0,);
}
}
// #endregion
return new vscode.TextEdit(edit.range, edit.newText.replaceAll('\t', ' '.repeat(tabSize as number)));
}));
}
action.edit = newEdit;
};