volar-plugins icon indicating copy to clipboard operation
volar-plugins copied to clipboard

Correctly indent output of code actions

Open zardoy opened this issue 2 years ago • 3 comments

Fixes https://github.com/vuejs/language-tools/issues/2425

As I understand its not possible to pass format options from the client, so why don't use defaults when options are missing?

But there is another problem: since virtual file is wrapped in function, it always adds leading identation, so I think we need patch output to dedent edits (and patch renameLocation). What do you think?

zardoy avatar Mar 13 '23 19:03 zardoy

Hi!

First of all, I think format code settings should always be merged, I'm not sure how requests happen there, but it already happens for vscode builtin extension https://github.com/microsoft/TypeScript/blob/a6ba2e735d379e1adbca06c82f09f182b6bb469e/src/server/editorServices.ts#L1041

Secondly, I think I've finally figured out a way to output code actions output with correct indentation.

I've done following testing (extract to type alias):

<script setup lang="ts">
const a: true
if (a) {
    const b: true
}
const c: {
    (a): any
} = a => a // also toggle braces action
</script>

and the same with initial indent.

Can I add automatic tests for these in volar tests?

Of course you might ask why code actions should output correct indentation? Well, this will help people that don't use formatter for some reason, and also I this will easy most other workflows (eg prettier cant format code with syntax, but TS code actions work)

I think I'm going to open a few more PRs to improve builtin TS code actions user experience, if you don't mind...

Update: Forgot to write solution explanation:

The text changes patching is based on how TS detects indentation: https://github.com/microsoft/TypeScript/blob/7c89eaa13172f74abaae35dbf144d12673d493dc/src/services/textChanges.ts#L1284 (probably we could also patch output of SmartIndenter, but as I see we don't patch typescripts methods here, correct me if I'm wrong). SmartIndenter just gives base indentation + extra indentation for block that starts on different line

zardoy avatar Mar 16 '23 19:03 zardoy

I think this PR scope should be reduced to handle default formatting options, and open a new PR for patching indent.

I am still looking for a more suitable solution than patching indent. Patching indent seems to need to deal with many edge cases, such as how to correctly support volar.format.initialIndent setting.

Yes of course you can add tests, but I'm a little bit unhappy with the current test and will probably refactor it.

johnsoncodehk avatar Mar 24 '23 00:03 johnsoncodehk

I think this PR scope should be reduced to handle default formatting options, and open a new PR for patching indent.

Can I remove handling default formatting options here instead? I more interested in seeing correct output of code actions. And these changes reflect PR's title, but defaults indeed not.

Patching indent seems to need to deal with many edge cases, such as how to correctly support volar.format.initialIndent setting.

It supports initial indent codestyle, you can check yourself. What more suitable solution you can think of?

zardoy avatar Mar 24 '23 00:03 zardoy

Sorry for the delay on this! Due to the changes in Vue language tools 2.0, the distinction between semantic server and syntactic server mentioned in https://github.com/vuejs/language-tools/pull/2498#issuecomment-1466557262 is no longer applicable. Now, the formatting options are shared as implemented in https://github.com/volarjs/services/commit/5987cf8d7dd7a2302197bec4f383073ccf6f5a28.

johnsoncodehk avatar May 09 '24 23:05 johnsoncodehk