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

virtual_documents customization #416

Merged
merged 8 commits into from
Dec 8, 2020

Conversation

fcollonval
Copy link
Contributor

@fcollonval fcollonval commented Dec 6, 2020

References

Fixes #353

Code changes

Move the ".virtual_documents" definition to a configuration in LanguageServerManager. Its default value is kept
but that default value can be overwritten by the environment variable JP_LSP_VIRTUAL_DIR

User-facing changes

None - except if they must know about the virtual document folder.

Backwards-incompatible changes

None

Chores

  • linted
  • tested
  • documented
  • changelog entry

@bollwyvl
Copy link
Collaborator

bollwyvl commented Dec 6, 2020

Good stuff!

It's unreliable to use os.path stuff for URIs because windows... pathlib.Path.as_uri() might be the better approach (we basically don't support py35 anymore), or fudge it and just use "/".join(), though then you have to worry about trailing slashes, etc.

The hard-coded string .virtual_documents may also appear in some other places as well, maybe the listener.

Anyhow, we would probably need a test of this being configured and (ab)used by the front end... e.g. spaces, non-ASCII, etc. Errors in this part of the (st|h)ack are extremely hard for end users to reason about, as the errors end up coming from the language servers talk about a folder that they might not even know about...

@fcollonval
Copy link
Contributor Author

Thanks @bollwyvl for the review

After a second look on the code and to take into account your remarks, do you think the following is a good idea:

virtual_documents_uri =  normalized_uri(pathlib.Path(contents.root_dir) / manager.virtual_documents_dir)

@bollwyvl
Copy link
Collaborator

bollwyvl commented Dec 7, 2020 via email

@fcollonval
Copy link
Contributor Author

Sorry for the useless small commits at the end.
Don't hesitate to squash & merge this one.

@krassowski krassowski merged commit 11ace76 into jupyter-lsp:master Dec 8, 2020
@krassowski
Copy link
Member

Thank you!

@fcollonval fcollonval deleted the fcollonval/issue353 branch December 8, 2020 20:18
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

virtual_documents customization
3 participants