-
-
Notifications
You must be signed in to change notification settings - Fork 1k
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
LSP Activates On Workspace Files, LSP Document Open/Close #1879
LSP Activates On Workspace Files, LSP Document Open/Close #1879
Conversation
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Real solid work @thecoolwinter! 👍
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Amazing PR! 🎉 Just a few updates needed in the docs 👍
CodeEdit/Features/CEWorkspace/Models/CEWorkspaceFileManager+FileManagement.swift
Outdated
Show resolved
Hide resolved
CodeEdit/Features/LSP/LanguageServer/Capabilities/LanguageServer+DocumentSync.swift
Outdated
Show resolved
Hide resolved
CodeEdit/Features/LSP/LanguageServer/Capabilities/LanguageServer+DocumentSync.swift
Outdated
Show resolved
Hide resolved
CodeEdit/Features/LSP/LanguageServer/Capabilities/LanguageServer+DocumentSync.swift
Outdated
Show resolved
Hide resolved
Sorry @austincondiff need another approval, fixed a failing test related to a subtle bug in #1840. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Great progress! Left some a small amount of comments
CodeEdit/Features/Documents/WorkspaceDocument/WorkspaceDocument.swift
Outdated
Show resolved
Hide resolved
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
LGTM, i agree with the points of @matthijseikelenboom
ecd03c8
Tracking issue for the commented-out event and notification handlers: #1888 |
Description
Main changes:
LanguageServer
type to use the existing workspace file management system.Misc details:
LanguageServer
methods to read more like sentences. Eg: instead ofserver.requestDocumentHighlight(document: document)
an API consumer would readserver. requestHighlight(for: document)
CodeFileDocument
class. A new methodgetLanguage()
guesses the language for the document unless it has been overriden.LazyService
dep injection property wrapper that is the same asService
but evaluates the service lazily. See usage.URL
extensions to one folder.LanguageServer
struct to a class. It's a long-living value that we'll need to reference in things like a text coordinator so a class makes more sense here. @FastestMolasses and I have had a bit of a back and forth on this. Originally it was made a struct to make async/await easier. So I investigated making itSendable
to fulfill that goal. However it needs to manage state like opened files, which breaks the immutability of sendable values.Related Issues
N/A
Checklist