Skip to content

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

Merged
sdedic merged 2 commits intoapache:masterfrom
sdedic:lsp/small-fixes
Nov 18, 2024
Merged

Use tripleslash file:/// URIs uniformly. Avoid null text in TextEdits.#7883
sdedic merged 2 commits intoapache:masterfrom
sdedic:lsp/small-fixes

Conversation

@sdedic
Copy link
Copy Markdown
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 labels Oct 18, 2024
@sdedic sdedic added this to the NB24 milestone Oct 18, 2024
@sdedic sdedic requested review from dbalek and jhorvath October 18, 2024 11:51
@sdedic sdedic self-assigned this Oct 18, 2024
@ebarboni ebarboni modified the milestones: NB24, NB25 Oct 22, 2024
@sdedic sdedic requested a review from lahodaj October 23, 2024 08:35
@lahodaj
Copy link
Copy Markdown
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
Copy Markdown
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.

lahodaj
lahodaj approved these changes Oct 25, 2024
View reviewed changes
Copy link
Copy Markdown
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
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Reviewers

@dbalek dbalek dbalek approved these changes

@lahodaj lahodaj lahodaj approved these changes

@jhorvath jhorvath Awaiting requested review from jhorvath

Assignees

@sdedic sdedic

Labels

LSP [ci] enable Language Server Protocol tests VSCode Extension

Projects

None yet

Milestone

NB25

Development

Successfully merging this pull request may close these issues.

4 participants

@sdedic @lahodaj @dbalek @ebarboni