Skip to content

Add on type conversion of tab char to spaces#396

Closed
evidolob wants to merge 1 commit into
redhat-developer:mainfrom
evidolob:convert-tab-to-spaces
Closed

Add on type conversion of tab char to spaces#396
evidolob wants to merge 1 commit into
redhat-developer:mainfrom
evidolob:convert-tab-to-spaces

Conversation

@evidolob
Copy link
Copy Markdown
Collaborator

What does this PR do?

Add on type conversion of tab char to spaces

What issues does this PR fix or reference?

#244

Is it tested? How?

With unit test. Unfortunately vscode doesn't trigger onTypeFormatting when \\t character is typed, maybe I need to report an issue for it.

Signed-off-by: Yevhen Vydolob <yvydolob@redhat.com>
@evidolob evidolob requested a review from JPinkney January 20, 2021 08:53
@evidolob evidolob self-assigned this Jan 20, 2021
@evidolob
Copy link
Copy Markdown
Collaborator Author

@mickaelistria Can you check this PR, as I won't able to test it in vscode?

@coveralls
Copy link
Copy Markdown

coveralls commented Jan 20, 2021

Coverage Status

Coverage increased (+0.3%) to 76.427% when pulling 639ae4d on evidolob:convert-tab-to-spaces into d35e49a on redhat-developer:master.

@mickaelistria
Copy link
Copy Markdown
Contributor

Unfortunately, LSP4E doesn't yet support onTypeFormatting ( https://bugs.eclipse.org/bugs/show_bug.cgi?id=537154 ) so I'm not able to test it either at the moment...

@gorkem
Copy link
Copy Markdown
Collaborator

gorkem commented Jun 25, 2023

Closing as the original issue is closed and this is no longer worked on

@gorkem gorkem closed this Jun 25, 2023
@mickaelistria
Copy link
Copy Markdown
Contributor

Can someone ( @msivasubramaniaan maybe? ) please reopen this issue and consider reviewing/merging such code? This feature is extremely helpful for editors that do not have particular language-specific indentation configuration built-in, and allow to get proper Yaml directly with the LS (without need to configure other editor parts).

}
}

if (params.ch === '\t' && params.options.insertSpaces) {
Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

I think the condition should be refined so that the replacement only happens before the line payload (ie no previous character on line or all previous chars on line are whitespaces)

@datho7561
Copy link
Copy Markdown
Contributor

I managed to rebase this PR locally and get the tests passing.

Unfortunately vscode doesn't trigger onTypeFormatting when \t character is typed, maybe I need to report an issue for it.

I started digging into the VS Code codebase to try and see why tab doesn't work as the trigger character, and didn't find any obvious reason. Space and newline work fine.

I'll file this upstream; it might take a while because they likely want a minimal reproducer. Until the issue is resolved, this change won't change anything in VS Code. So, we'll need to test this feature against a different LSP client.

@datho7561
Copy link
Copy Markdown
Contributor

See microsoft/vscode#247651

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Projects

None yet

Development

Successfully merging this pull request may close these issues.

5 participants