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

Use tripleslash file:/// URIs uniformly. Avoid null text in TextEdits. #7883

Merged
merged 2 commits into from
Nov 18, 2024

Conversation

sdedic
Copy link
Member

@sdedic sdedic commented Oct 18, 2024

The PR provides two small fixes:

  • Utils.toUri returns file:///path/somewhere URIs even though native translation returns "file:/path/somewhere". Incoming URIs seem to use file:/// consistently, so we should do as well, so they can compare as strings
  • When converting from lsp API TextEdit to lsp4j protocol structure, make sure that the text property is always non-null

@sdedic sdedic added LSP [ci] enable Language Server Protocol tests VSCode Extension [ci] enable VSCode Extension tests labels Oct 18, 2024
@sdedic sdedic added this to the NB24 milestone Oct 18, 2024
@sdedic sdedic self-assigned this Oct 18, 2024
@ebarboni ebarboni modified the milestones: NB24, NB25 Oct 22, 2024
@lahodaj
Copy link
Contributor

lahodaj commented Oct 23, 2024

I think I would appreciate more details on the tricks played for Windows URIs - what exactly is the purpose? Thanks!

@sdedic
Copy link
Member Author

sdedic commented Oct 23, 2024

I think I would appreciate more details on the tricks played for Windows URIs - what exactly is the purpose? Thanks!

Windows URLs, which are not network mounts (file://hostname/mount/path) use file:/C:/path (where "C" is the drive letter). As the code tries to replace everything which starts with file: except file:///, I wanted to avoid damaging file:/C:/path URIs. That's all.

Copy link
Contributor

@lahodaj lahodaj left a comment

Choose a reason for hiding this comment

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

Looks good to me.

@sdedic sdedic merged commit f18acc6 into apache:master Nov 18, 2024
32 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
LSP [ci] enable Language Server Protocol tests VSCode Extension [ci] enable VSCode Extension tests
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants