Extract out an interface for our LSP miscellaneous files workspace #78401
Conversation
This will allow us to substitute in a different implementation in our real server.
ghost
added
Area-IDE
untriaged
| { | ||
| TextDocument? AddMiscellaneousDocument(Uri uri, SourceText documentText, string languageId, ILspLogger logger); | ||
| void TryRemoveMiscellaneousDocument(Uri uri, bool removeFromMetadataWorkspace); | ||
| } |
There was a problem hiding this comment.
up to you, but i wouldn't mind docs.
Also, being an ILspWorkspace and an ILspService seems odd. I don't really understand the role this plays as both those types.
There was a problem hiding this comment.
also, as an interface, do we expect multiple impls of this? i'd love an explanation of that. thanks!
There was a problem hiding this comment.
I think the misc workspace is an ILspService currently, that's why LspWorkspaceManagerFactory can call GetService<LspMiscellaneousFilesWorkspace>().
The plan is for FileBasedProgramWorkspace to also implement the new interface. Most likely we are going to have an opt-out switch for file-based programs support, which simply makes it so the "existing" misc files workspace gets used instead of the "new" FileBasedProgramWorkspace.
|
|
||
| internal interface ILspMiscellaneousFilesWorkspaceProvider : ILspService | ||
| { | ||
| ILspMiscellaneousFilesWorkspace CreateLspMiscellaneousFilesWorkspace(ILspServices lspServices, HostServices hostServices); |
There was a problem hiding this comment.
i understand this 'provider' being an ILspService. but if it makes ILspXXXWorkspaces, is the ILspXXXWorkspace an ILspService itself? Seems confusing to me.
There was a problem hiding this comment.
Maybe it would make more sense for the LspWorkspaceManagerFactory to simply ask for a ILspMiscellaneousFilesWorkspaceProvider, rather than having to register the workspace as its own service and have the ManagerFactory ask for that.
Wouldn't mind explanation (either in PR, in docs, or offline) of what the different impl woudl be. And what constitutes "our real server" :) |
jasonmalinowski
removed
the
untriaged
This coupling can cause challenges, so the easy change is to add a Workspace property which provides the workspace. This renames the ILspMiscellaneousFilesWorkspace interface to add a Provider suffix, and the existing Provider now becomes a ProviderFactory.
jasonmalinowski
force-pushed
the
extract-lsp-miscellaneous-files-interface
branch
from
125efcc to
aa6439d
Compare
| /// </summary> | ||
| Workspace Workspace { get; } | ||
| TextDocument? AddMiscellaneousDocument(Uri uri, SourceText documentText, string languageId, ILspLogger logger); | ||
| void TryRemoveMiscellaneousDocument(Uri uri, bool removeFromMetadataWorkspace); |
There was a problem hiding this comment.
doc'ing both of the above methods would be good. for example, why does AddMiscDoc return nullable?
Why is it TryRemoveMiscDoc? Odd that it doesn't return a boolean.
| [method: ImportingConstructor] | ||
| [method: Obsolete(MefConstruction.ImportingConstructorMessage, error: true)] | ||
| internal sealed class LspMiscellaneousFilesWorkspaceProvider(IMetadataAsSourceFileService metadataAsSourceFileService) : ILspService | ||
| internal sealed class LspMiscellaneousFilesWorkspaceProvider(ILspServices lspServices, IMetadataAsSourceFileService metadataAsSourceFileService, HostServices hostServices) |
There was a problem hiding this comment.
nit. when PC is this long, consider wrapping.
| [method: Obsolete(MefConstruction.ImportingConstructorMessage, error: true)] | ||
| internal sealed class LspMiscellaneousFilesWorkspaceProvider(IMetadataAsSourceFileService metadataAsSourceFileService) : ILspService | ||
| internal sealed class LspMiscellaneousFilesWorkspaceProvider(ILspServices lspServices, IMetadataAsSourceFileService metadataAsSourceFileService, HostServices hostServices) | ||
| : Workspace(hostServices, WorkspaceKind.MiscellaneousFiles), ILspMiscellaneousFilesWorkspaceProvider, ILspWorkspace |
There was a problem hiding this comment.
being a misc workspace provider and a workspace at the same time is breaking my brain.
| public bool SupportsMutation => true; | ||
|
|
||
| // For this implementation, it's easiest to just have it inherit from Workspace, so we'll just return this. | ||
| public Workspace Workspace => this; |
| var container = new StaticSourceTextContainer(documentText); | ||
| if (metadataAsSourceFileService.TryAddDocumentToWorkspace(documentFilePath, container, out var documentId)) | ||
| { | ||
| var metadataWorkspace = metadataAsSourceFileService.TryGetWorkspace(); |
There was a problem hiding this comment.
maybe have the TryAddDocToWorkspace return the worskpace as well? to avoid this?
|
|
||
| var projectInfo = MiscellaneousFileUtilities.CreateMiscellaneousProjectInfoForDocument( | ||
| this, documentFilePath, sourceTextLoader, languageInformation, documentText.ChecksumAlgorithm, Services.SolutionServices, []); | ||
| OnProjectAdded(projectInfo); |
There was a problem hiding this comment.
never very clear on locking around all of this work.
| var documentFilePath = ProtocolConversions.GetDocumentFilePathFromUri(uri); | ||
| if (removeFromMetadataWorkspace && metadataAsSourceFileService.TryRemoveDocumentFromWorkspace(documentFilePath)) | ||
| { | ||
| return; |
There was a problem hiding this comment.
i feel like i want to return a value from TryRemoveMiscDoc and have the callers contract call on it.
| } | ||
|
|
||
| // Also remove the project - we always create a new project for each misc file we add | ||
| // so it should never have other documents in it. |
|
|
||
| // We'll only ever have a single document matching this URI in the misc solution. | ||
| var matchingDocument = CurrentSolution.GetDocumentIds(uri).SingleOrDefault(); | ||
| if (matchingDocument != null) |
There was a problem hiding this comment.
curious when this is null. i don't feel like i ahve a good sense of the invariants we have in this space. feels loosey goosey.
| public ILspMiscellaneousFilesWorkspaceProvider CreateLspMiscellaneousFilesWorkspaceProvider(ILspServices lspServices, HostServices hostServices) | ||
| { | ||
| return new LspMiscellaneousFilesWorkspaceProvider(lspServices, metadataAsSourceFileService, hostServices); |
There was a problem hiding this comment.
do we expect to have other impls of ILspMiscellaneousFilesWorkspaceProviderFactory?
|
Added a few more comments. But still ok with this going in. |
This will allow us to substitute in a different implementation in our real server.