ipatalas / vscode-postfix-ts

Postfix notation for TypeScript/Javascript - extension for VS Code
MIT License
158 stars 43 forks source link

Fix incorrect multiline indentation inserting non-snippet text #83

Closed zardoy closed 2 years ago

zardoy commented 2 years ago

I added failing tests to illustrate the problem.

I tried different approaches, and it seems only insertText of completion and editor.insertText does whitespace adjusting in VSCode. First one causes editor scroll jump higher, and a second one messes up with undo stack. So I tried to do whitespace normalization manually, however:

  1. It doesn't seem to be logical that we first remove all whitespace characters (adjustMultilineIndentation) and then add them back (code that adds this pr).
  2. There is an issue with .not:
    if (
        a && (b &&
        a
        .a
        .b).not
    )  {}
codecov[bot] commented 2 years ago

Codecov Report

Base: 97.84% // Head: 97.81% // Decreases project coverage by -0.03% :warning:

Coverage data is based on head (b04272e) compared to base (b83e0d2). Patch coverage: 98.79% of modified lines in pull request are covered.

Additional details and impacted files ```diff @@ Coverage Diff @@ ## develop #83 +/- ## =========================================== - Coverage 97.84% 97.81% -0.04% =========================================== Files 21 21 Lines 1209 1236 +27 Branches 225 226 +1 =========================================== + Hits 1183 1209 +26 - Misses 25 26 +1 Partials 1 1 ``` | [Impacted Files](https://codecov.io/gh/ipatalas/vscode-postfix-ts/pull/83?src=pr&el=tree&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=Ireneusz+Patalas) | Coverage Δ | | |---|---|---| | [src/completionItemBuilder.ts](https://codecov.io/gh/ipatalas/vscode-postfix-ts/pull/83/diff?src=pr&el=tree&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=Ireneusz+Patalas#diff-c3JjL2NvbXBsZXRpb25JdGVtQnVpbGRlci50cw==) | `95.83% <93.75%> (-0.57%)` | :arrow_down: | | [src/postfixCompletionProvider.ts](https://codecov.io/gh/ipatalas/vscode-postfix-ts/pull/83/diff?src=pr&el=tree&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=Ireneusz+Patalas#diff-c3JjL3Bvc3RmaXhDb21wbGV0aW9uUHJvdmlkZXIudHM=) | `93.46% <100.00%> (+0.17%)` | :arrow_up: | | [src/templates/baseTemplates.ts](https://codecov.io/gh/ipatalas/vscode-postfix-ts/pull/83/diff?src=pr&el=tree&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=Ireneusz+Patalas#diff-c3JjL3RlbXBsYXRlcy9iYXNlVGVtcGxhdGVzLnRz) | `100.00% <100.00%> (ø)` | | | [src/templates/castTemplates.ts](https://codecov.io/gh/ipatalas/vscode-postfix-ts/pull/83/diff?src=pr&el=tree&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=Ireneusz+Patalas#diff-c3JjL3RlbXBsYXRlcy9jYXN0VGVtcGxhdGVzLnRz) | `100.00% <100.00%> (ø)` | | | [src/templates/consoleTemplates.ts](https://codecov.io/gh/ipatalas/vscode-postfix-ts/pull/83/diff?src=pr&el=tree&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=Ireneusz+Patalas#diff-c3JjL3RlbXBsYXRlcy9jb25zb2xlVGVtcGxhdGVzLnRz) | `100.00% <100.00%> (ø)` | | | [src/templates/customTemplate.ts](https://codecov.io/gh/ipatalas/vscode-postfix-ts/pull/83/diff?src=pr&el=tree&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=Ireneusz+Patalas#diff-c3JjL3RlbXBsYXRlcy9jdXN0b21UZW1wbGF0ZS50cw==) | `100.00% <100.00%> (ø)` | | | [src/templates/forTemplates.ts](https://codecov.io/gh/ipatalas/vscode-postfix-ts/pull/83/diff?src=pr&el=tree&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=Ireneusz+Patalas#diff-c3JjL3RlbXBsYXRlcy9mb3JUZW1wbGF0ZXMudHM=) | `100.00% <100.00%> (ø)` | | | [src/templates/ifTemplates.ts](https://codecov.io/gh/ipatalas/vscode-postfix-ts/pull/83/diff?src=pr&el=tree&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=Ireneusz+Patalas#diff-c3JjL3RlbXBsYXRlcy9pZlRlbXBsYXRlcy50cw==) | `100.00% <100.00%> (ø)` | | | [src/templates/newTemplate.ts](https://codecov.io/gh/ipatalas/vscode-postfix-ts/pull/83/diff?src=pr&el=tree&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=Ireneusz+Patalas#diff-c3JjL3RlbXBsYXRlcy9uZXdUZW1wbGF0ZS50cw==) | `100.00% <100.00%> (ø)` | | | [src/templates/notTemplate.ts](https://codecov.io/gh/ipatalas/vscode-postfix-ts/pull/83/diff?src=pr&el=tree&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=Ireneusz+Patalas#diff-c3JjL3RlbXBsYXRlcy9ub3RUZW1wbGF0ZS50cw==) | `95.38% <100.00%> (+0.07%)` | :arrow_up: | | ... and [4 more](https://codecov.io/gh/ipatalas/vscode-postfix-ts/pull/83/diff?src=pr&el=tree-more&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=Ireneusz+Patalas) | | Help us with your feedback. Take ten seconds to tell us [how you rate us](https://about.codecov.io/nps?utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=Ireneusz+Patalas). Have a feature suggestion? [Share it here.](https://app.codecov.io/gh/feedback/?utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=Ireneusz+Patalas)

:umbrella: View full report at Codecov.
:loudspeaker: Do you have feedback about the report comment? Let us know in this issue.

zardoy commented 2 years ago

I also checked that my proposed workaround for snippets in https://github.com/ipatalas/vscode-postfix-ts/pull/73#issuecomment-1249724849 doesn't do whitespace adjustment as well.

Just leaving a reference: https://github.com/microsoft/vscode/blob/058e70bff799899843e110b8d263788dc3aa83be/src/vs/editor/contrib/snippet/browser/snippetSession.ts#L388 it seems to be straightforward

zardoy commented 2 years ago

@ipatalas Sorry I was completely killed last evening and as I see didn't describe things clearly here.

So with https://github.com/ipatalas/vscode-postfix-ts/pull/73 I broke inserting text for non-snippet templates (like console or return) when node had leading whitespace characters. As I described in the body, there is no reliable way to reuse this logic from vscode, but we can do it manually since its not complicated.

I finally managed to provide my final solution to this. The only issue is with .not template:

tl;dr I do not understand why https://github.com/ipatalas/vscode-postfix-ts/blob/eb7f989d7c3b9f5d2641f75007a29367cfd3ff98/src/utils/invert-expression.ts#L36 this is being called on text that is returned only in some cases. Why we don't normalize final text result?

Also let me know what you think about it, as it touches every template.

ipatalas commented 2 years ago

Hi, sorry, I already had a very simple solution for that few hours ago but had to go offline :) Anyway I think that fixes the problem: https://github.com/ipatalas/vscode-postfix-ts/commit/c66c8f6213244cd4b067bc8bb6f7b5f6ed48037f All tests are green and it's one liner fix :D The thing is that I was already trying to fix whitespace problems with that adjustMultilineIndentation function. For snippet strings vscode does that on its own as you mentioned so now I just turned off this adjustMultilineIndentation for non-snippet completions and apparently it works quite well.

It has small issue with your .not example but this might be related to what you discovered in that invert-expression.ts. I don't remember why it's not fixing the output. Perhaps at the beginning it was working fine, the code there was less complex and then it grew but this stayed because nobody complained ;)

ipatalas commented 2 years ago

Kind of fixed .not partially: bug-83-not

The other alternative doesn't work perfectly (adds one extra new line which shouldn't be there) but maybe I'll find a way. Actually this fix means removing some code - I like that :D Perhaps VS Code API has changed many things since this adjustMultilineIndentation was first introduced. It looks like it's no longer needed in most places.

zardoy commented 2 years ago

c66c8f6

Yeah, you might noticed that this PR has a bit more code changes, because I'm replicating VSCode's normalization behavior (that's why I'm wrapping output result, not code). Sorry if I overengineered this, didn't see 1 line solution here :)

All tests are green and it's one liner fix

Sorry, but there are still cases that it doesn't fix:

{
    "name": "custom",
    "body": "1\n\t1\n{{expr}}",
    "when": [
        "identifier",
        "expression",
        "function-call"
    ]
},

Or even better, without {{expr}}: 1\n\t1\n1

Examples:

    a
    .b
    .c{custom}

Correct (just add $0 to the end):

    1
        1
    a
    .b
    .c

Incorrect:

    1
    1
a
    .b
    .c
ipatalas commented 2 years ago

It's not a final solution, i didn't push last commit because I'm still trying to figure this out. Anyway I'm advocating this solution to avoid having two places that adjust whitespaces. I'd rather remove the old solution (which I sort of did) and then start over with the fixes. Otherwise it will be hard to maintain if two functions are doing more or less the same in two different places.

ipatalas commented 2 years ago

All right, fixed that first problem: bug-83-not-v2

The template is clearly reversible for both alternatives with no side effects. It still won't work with your custom template example, it's another story because the template replacement is multi-line and contains whitespaces. That will be next step where this PR's code might come handy. I reviewed that quickly and the idea itself looks pretty good, I had the same idea with IndentInfo object to pass additional information.

ipatalas commented 2 years ago

Ok, pushed the changes and now .not should work much better with those whitespaces. Be careful when resolving conflicts as we both did changes in the same lines. I'll try to review and test this PR tomorrow and then it would also be done for custom templates 👍🏻

zardoy commented 2 years ago

Sorry for the delay, I somehow missed the fact that pr with .not template was merged. I also added a multiline test with custom template that I mentioned above. Let me know what you think.

zardoy commented 2 years ago

@ipatalas Hey! It'd be great if you published this fix on marketplace :)

By the way, I'm just curious do you personally use this extension, or you moved away from TS development ?

ipatalas commented 2 years ago

@ipatalas Hey! It'd be great if you published this fix on marketplace :)

Hey, sorry, somehow thought I did that already. Anyway, just published.

By the way, I'm just curious do you personally use this extension, or you moved away from TS development ?

Unfortunately not at the moment. I used to be a fullstack and then I was using it extensively in day to day work but currently I do almost only BE at my current project hence I don't have a need to use it. Of course I have it installed and ready to be used whenever I have anything JS/TS related to do but it happens only once or twice a month.