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

file path auto completion add \ in path string #373

Closed
troubadour-hell opened this issue Apr 26, 2023 · 9 comments · Fixed by #497
Closed

file path auto completion add \ in path string #373

troubadour-hell opened this issue Apr 26, 2023 · 9 comments · Fixed by #497
Labels
bug Something isn't working
Milestone

Comments

@troubadour-hell
Copy link

troubadour-hell commented Apr 26, 2023

Hello!I use jupyterlab-lsp with python-lsp-server. When I try to type in a file path, the auto-completion adds an \ before the last /, and it can't suggest the contents in this directory.
image
image
I find this:


When I Removed "\\" from this line, it worked well. I'm not sure why it do this...

@ccordoba12
Copy link
Member

Hey @troubadour-hell, thanks for reporting. I think this problem depends on the way JupyterLab-LSP processes paths that come from this server.

@krassowski, thoughts about this?

@krassowski
Copy link
Contributor

For background, these changes were introduced 3 months ago in #337

I do not understand why the slashes there are escaped; my first expectation would be that no escaping should be needed on the server level and if escaping is needed for display then it is the clients' responsibility. Can you elaborate on the reasoning?

@ccordoba12
Copy link
Member

Here you can find the reasoning: palantir/python-language-server#762 (see the PR description).

@krassowski
Copy link
Contributor

So the rationale is to prevent conflicts with snippet grammar which uses escapes. In that case it should be only escaped when CompletionClientCapabilities include completionItem.snippetSupport.

@ccordoba12
Copy link
Member

Yep, I agree with that. Could you submit a PR for it?

@rchl
Copy link
Contributor

rchl commented May 6, 2023

I might not have that much context about this issue and I don't know what kind of values can be provide through those completions but I would think that either:
a) those completions can contain snippet replacements in which case it wouldn't really be correct to provide those to a client that doesn't support snippets. So in that case those should always be escaped if client supports snippets and just omitted if it doesn't.
b) those completions never contain snippet replacements in which case those should just not be marked as having a snippet type and never escaped.

@ccordoba12
Copy link
Member

a) those completions can contain snippet replacements in which case it wouldn't really be correct to provide those to a client that doesn't support snippets. So in that case those should always be escaped if client supports snippets and just omitted if it doesn't.

Right, that's what @krassowski suggested we should do to solve this problem.

@ccordoba12 ccordoba12 added this to the v1.9.1 milestone Dec 7, 2023
@ccordoba12 ccordoba12 added the bug Something isn't working label Dec 7, 2023
@RocketRide9
Copy link

do I understand right that this issue is fixed in 1.10? I can reproduce it using helix editor. I previously reported this issue to them: helix-editor/helix#9575.

I checked version this way:

(.venv) ⬢ image_compare pylsp --version
pylsp v1.10.0

@pascalkuthe
Copy link

Helix supports snippet so this fix does not apply to us. The LSP standard does not List \/ as a valid escape in snippets.

\/ is only a valid escape inside a regex. So in most situations \/ will be printer verbatim by VSCode and helix. Most likely printing \/ instead of / should just be reverted.

If you do actually use a regex in your snippets then you need to retroactively escape slashes when you generate the regex (or control that using somekind of flag)

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
bug Something isn't working
Projects
None yet
Development

Successfully merging a pull request may close this issue.

6 participants