investigate escaping jumping to definition (and other features) with spaces

[bollwyvl]

References

• fixes Jump to definition handling spaces in URI inconsistently #403

Code changes

• atest
  • demonstrate a reproducible failure (bb39801)
  • rework some globals to be set by Variables.robot (can be overridden from CLI)
  • move more lab config into JSON, launch without shell=True
• @krassowski/jupyterlab-lsp
  • remove a number of (decode|encode)URI calls
  • always encodeURI the document path
• @krassowski/jupyterlab_go_to_definition
  • rename IGlobalJump.uri to IGlobalJump.contents_path
  • add some more information to the docstrings
  • sweep for more references to URIs that are actually contents paths
• consider any additional ways to prevent this in the future
  • e.g. more normalization at boundaries?
  • find/build/contribute a better URI library (Refactor/replace URI utilities with dedicated type #410)

User-facing changes

Should reliably Jump between files in folders with spaces and special characters, including when jupyter lab is launched from directories containing spaces, especially on windows.

Backwards-incompatible changes

• @krassowski/jupyterlab_go_to_definition
  • IGlobalJump.uri renamed to IGlobalJump.contents_path

Chores

• linted
• tested
• documented
• changelog entry

[bollwyvl]

Ha: failed to even start the acceptance tests on windows, so haven't yet reproduced (even though it's failing).

We're generally very careful about paths, but the way we start jupyter under test is quite kludgy... I'll look at doing more of it with a json file which is less sensitive to paths/escaping.

[avaissi]

Anything here I could help with? Maybe to investigate the issue more for root cause, if not already found

[bollwyvl]

Unfortunately, it's a bit in the weeds! I have a plan, it's just going to take some time. Spaces, permissions, and unicode should be table stakes for acceptance testing anything, but it's easy to forget. Thanks again for the report!

[krassowski]

I wonder if the path addition should not be pushed further down to the actual robot tests. I think that we already have one notebook with space in the name, probably just not the one testing jump functions. Though having this as a top dir would be convenient indeed.

[bollwyvl]

Indeed! I was kind of tired.

Moved it (and a number of other paths) down into Variables.robot... they can be overridden with:

python -m scripts.atest --variable "NOTEBOOK DIR NAME:🐶 🐶 🐶"

would yield:

atest/output/linux_37_1/home/🐶 🐶 🐶/

[bollwyvl]

Yeah the issue is "spaces in the rootUri break everything". Including starting the test server, at present!

[bollwyvl]

gah... the formatting of the flake8 messages now leads with the code, e.g. F821 undefined name. Will try with title*=[<the diagnostic>], as who knows...

[bollwyvl]

Well, still didn't get to reproducing #403 on windows (or at least hasn't finished), but otherwise good progress...

[bollwyvl]

Great news! I can reproduce the error!

Hopefully we'll get just that fail for all of the tests, and I can add the decodeURI part!

[bollwyvl]

Actually had a look deeper: turns out the IGlobalJump.uri is not a URI at all, and has always been the more "humane" Jupyter contents path. This in turn gets URL encoded when it actually hits the ContentsManager, as HTTP is ASCII, like it or lump it.

A few changes throughout were required to normalize this behavior (86f5abe), and make the naming more accurate (89102dd, breaking change), and linting (2e43577) but it should hopefully now be more robust overall, and address some other odd issues I had seen but hadn't narrowed down to this root cause.

On #278 I briefly toyed with using an existing URI library, but ran into some issues... it may be worth revisiting, either:

• adopting somebody's,
• getting one into lab/lumino
• at worst rolling our own
  • which can already handle things like URI.toContentsPath(), etc.

At any rate, whatever we use, we should probably never treat a URI as a plain-old-string, which would prvent these kinds of issues, as well as double encode, etc.

Going to let a full local clean build run while CI is chugging, and might not address any more this evening... good times!

[bollwyvl]

Local run looks good, and 89102dd has test wins (aside from lint) on all the platforms:
https://github.com/krassowski/jupyterlab-lsp/runs/1404308642

Taking off WIP, ready for review!

[krassowski]

Ok, looks great will merge once CI passes. Thank you!

[bollwyvl]

Yeah, if there is an issue, and it's just little old me that did the work, I just put the issue number, instead of The Work... I'm sure whatever state is in now (if you pushed) is fine! Very good though, to give shout outs new contributor prs, though!

[BugApe]

This problem also occurs when the filename contains "=", such as "NeuralTaskonomy_axis=1.ipynb", and renaming the variable will fail.

[krassowski]

What language server are you using?

[BugApe]

What language server are you using?

python

[krassowski]

@BugApe, which one:

• pyls
• pylsp
• jedi-ls
• pyright

? Could you open a new issue following bug report template?