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.

Sign up for GitHub

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

Jump to bottom

Omni sharp decouple #7936

Merged
merged 16 commits into from
Nov 22, 2022
Merged

Omni sharp decouple #7936

merged 16 commits into from
Nov 22, 2022

Conversation

ryanbrandenburg
Copy link
Contributor

Summary of the changes

  • In order to know what we're going to do with O#Plugin we need to first understand the surface area we need to expose to them. A lot of that is handles VIA O#Plugin.StrongName, but the non-internal references matter too.
  • We already have a tool for that though, I'm turning on the PublicAPI analyzer for RazorTooling.
  • Also made any API which isn't explicitly being used by O#Plugin internal to minimize the amount of promises we'll have to keep. If anyone knows of additional sources of "promises" that we've already made WRT API surface-area let me know.

Fixes part of #6945.

@ryanbrandenburg ryanbrandenburg requested review from a team as code owners November 18, 2022 23:04
ryanbrandenburg
ryanbrandenburg commented Nov 18, 2022
View reviewed changes
src/Razor/src/Microsoft.AspNetCore.Razor.LanguageServer.Common/LanguageServerConstants.cs
@@ -3,77 +3,77 @@

namespace Microsoft.AspNetCore.Razor.LanguageServer.Common;

public static class LanguageServerConstants
Copy link
Contributor Author

Choose a reason for hiding this comment

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

Recommend viewing with "?w=1", there was some formatting that still needed to happen after the namespace change.

davidwengier
davidwengier reviewed Nov 18, 2022
View reviewed changes
Copy link
Contributor

@davidwengier davidwengier left a comment

Choose a reason for hiding this comment

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

Haven't fully reviewed, and on my phone so I'm stopping, but maybe also left enough comments to start some discussions

.../Microsoft.AspNetCore.Razor.Microbenchmarks/LanguageServer/RazorCSharpFormattingBenchmark.cs Outdated Show resolved Hide resolved
src/Razor/src/Microsoft.AspNetCore.Razor.LanguageServer.Common/LanguageServerConstants.cs
public const int VSCompletionItemKindOffset = 118115;
public static class LanguageServerConstants
{
internal const int VSCompletionItemKindOffset = 118115;
Copy link
Contributor

Choose a reason for hiding this comment

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

This file is a great example of where I'd love to have a new DLL serve as an external access layer, so its much easier to see exactly what surface area is exposed, and also harder (if not impossible) to accidentally widen that area.

Of course that could come as a follow up, not in this PR, since this one does the hard work of at least identifying what that surface area is.

...pNetCore.Razor.OmniSharpPlugin.StrongNamed/DefaultOmniSharpProjectSnapshotManagerAccessor.cs Outdated Show resolved Hide resolved
...ft.AspNetCore.Razor.OmniSharpPlugin.StrongNamed/OmniSharpProjectSnapshotManagerDispatcher.cs Outdated Show resolved Hide resolved
src/Razor/src/Microsoft.AspNetCore.Razor.OmniSharpPlugin/MSBuildProjectManager.cs Outdated Show resolved Hide resolved
ryanbrandenburg
ryanbrandenburg commented Nov 21, 2022
View reviewed changes
...r/test/Microsoft.AspNetCore.Razor.LanguageServer.Test/Completion/TagHelperServiceTestBase.cs
@@ -25,9 +25,9 @@ public abstract class TagHelperServiceTestBase : LanguageServerTestBase
protected const string RazorFile = "test.razor";

protected TagHelperDescriptor[] DefaultTagHelpers { get; }
protected RazorTagHelperCompletionService RazorTagHelperCompletionService { get; }
internal RazorTagHelperCompletionService RazorTagHelperCompletionService { get; }
Copy link
Contributor Author

Choose a reason for hiding this comment

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

These changes are required because the type has become internal

davidwengier
davidwengier approved these changes Nov 21, 2022
View reviewed changes
Copy link
Contributor

@davidwengier davidwengier left a comment

Choose a reason for hiding this comment

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

Seems fine to me. As mentioned, I'd love a single project like an EA layer, to encapsulate this all together into one public API, rather than piecemeal on each project, and rather than mixing visibilities in a single type, but that can be done as a follow up.

...c/Microsoft.AspNetCore.Razor.LanguageServer.Common/Extensions/RazorCodeDocumentExtensions.cs Show resolved Hide resolved
@ryanbrandenburg ryanbrandenburg merged commit 4e6fc5a into dotnet:main Nov 22, 2022
@ryanbrandenburg ryanbrandenburg deleted the OmniSharpDecouple branch November 22, 2022 22:38
@ryanbrandenburg ryanbrandenburg mentioned this pull request Nov 23, 2022
Closed
allisonchou added a commit that referenced this pull request Dec 1, 2022
@allisonchou
Revert "Merge pull request #7936 from ryanbrandenburg/OmniSharpDecouple"
386a752 
This reverts commit 4e6fc5a, reversing
changes made to cad3ad5.
allisonchou added a commit that referenced this pull request Dec 1, 2022
@allisonchou
Revert "Merge pull request #7936 from ryanbrandenburg/OmniSharpDecoup…
b835344 
…le" (#7980)

This reverts commit 4e6fc5a, reversing
changes made to cad3ad5.
@davidwengier
Copy link
Contributor

FYI, this broke VS Mac as apparently WebTools calls RazorEditorFactoryService.TryGetDocumentTracker somehow. Something to check if/when this gets resurrected

ERROR [2022-12-05 14:20:46Z]: 
View failed to load System.AggregateException: One or more errors occurred. 
(Attempt by method 'Microsoft.WebTools.Languages.Razor.Core.RazorCodeGenerator.OnDocumentReady(Microsoft.WebTools.Languages.Html.Editor.Document.HtmlEditorDocument)' to access method 'Microsoft.VisualStudio.Editor.Razor.RazorEditorFactoryService.TryGetDocumentTracker(Microsoft.VisualStudio.Text.ITextBuffer, Microsoft.VisualStudio.Editor.Razor.VisualStudioDocumentTracker ByRef)' failed.) 
---> System.MethodAccessException: Attempt by method Microsoft.WebTools.Languages.Razor.Core.RazorCodeGenerator.OnDocumentReady(Microsoft.WebTools.Languages.Html.Editor.Document.HtmlEditorDocument)' to access method 'Microsoft.VisualStudio.Editor.Razor.RazorEditorFactoryService.TryGetDocumentTracker(Microsoft.VisualStudio.Text.ITextBuffer, Microsoft.VisualStudio.Editor.Razor.VisualStudioDocumentTracker ByRef)' failed. at Microsoft.WebTools.Languages.Razor.Core.RazorCodeGenerator.OnDocumentReady(HtmlEditorDocument htmlDocument) 
in /Users/runner/work/1/s/src/Languages/Razor/VS/Core/RazorCodeGenerator.cs:line 162 at Microsoft.WebTools.Languages.Shared.Editor.Services.ServiceManager.AdviseServiceAdded[T](IPropertyOwner propertyOwner, Action`1 callback) 
in /Users/runner/work/1/s/src/Languages/Shared/Editor/Services/ServiceManager.cs:line 41 at Microsoft.WebTools.Languages.Razor.Core.RazorCodeGenerator..ctor(RazorSyntaxFactsService syntaxFactsService, EditorSettingsManager editorSettingsManager, RazorEditorFactoryService editorFactoryService, ICompletionBroker completionBroker, IAsyncCompletionBroker asyncCompletionBroker, ITextBufferFactoryService textBufferFactoryService, ITextDifferencingSelectorService textDifferencingSelectorService, ITextBuffer buffer) 
in /Users/runner/work/1/s/src/Languages/Razor/VS/Core/RazorCodeGenerator.cs:line 151 at Microsoft.WebTools.Languages.Razor.Core.RazorCodeGenerator.Create(ITextBuffer buffer, RazorSyntaxFactsService syntaxFactsService, EditorSettingsManager editorSettingsManager, RazorEditorFactoryService editorFactoryService, ICompletionBroker completionBroker, IAsyncCompletionBroker asynccompletionBroker, ITextBufferFactoryService textBufferFactoryService, ITextDifferencingSelectorService textDifferencingSelectorService) 
in /Users/runner/work/1/s/src/Languages/Razor/VS/Core/RazorCodeGenerator.cs:line 115 at Microsoft.WebTools.Languages.Razor.Core.RazorCodeGeneratorProvider.Microsoft.WebTools.Languages.Html.Editor.Razor.IRazorCodeGeneratorProvider.CreateRazorCodeGenerator(ITextBuffer buffer, Version razorVersion, String physicalPath, String virtualPath) 
in /Users/runner/work/1/s/src/Languages/Razor/VS/Core/RazorCodeGeneratorProvider.cs:line 49 at Microsoft.WebTools.Languages.Html.Editor.Razor.RazorCodeGeneratorFactory.CreateCodeGenerator(ITextBuffer buffer, Version razorVersion, String physicalPath, String virtualPath) 
in /Users/runner/work/1/s/src/Languages/html/Editor/Razor/RazorCodeGeneratorFactory.cs:line 21 at Microsoft.WebTools.Languages.Html.Editor.ContentType.Handlers.RazorContentTypeHandler.OnHtmlDocumentCreated(HtmlEditorDocument document) 
in /Users/runner/work/1/s/src/Languages/html/Editor/ContentType/Handlers/RazorContentTypeHandler.cs:line 165 at Microsoft.WebTools.Languages.Shared.Editor.Services.ServiceManager.<>c__DisplayClass10_0`1.<AdviseServiceAdded>g__onServiceAdded|0(Object sender, ServiceManagerEventArgs eventArgs) 
in /Users/runner/work/1/s/src/Languages/Shared/Editor/Services/ServiceManager.cs:line 49 at Microsoft.WebTools.Languages.Shared.Editor.Services.ServiceManager.FireServiceAdded(Type serviceType, Object serviceInstance) 
in /Users/runner/work/1/s/src/Languages/Shared/Editor/Services/ServiceManager.cs:line 360 at Microsoft.WebTools.Languages.Shared.Editor.Services.ServiceManager.AddService[T](T serviceInstance) 
in /Users/runner/work/1/s/src/Languages/Shared/Editor/Services/ServiceManager.cs:line 329 at Microsoft.WebTools.Languages.Shared.Editor.Services.ServiceManager.AddService[T](T serviceInstance, IPropertyOwner propertyOwner) 
in /Users/runner/work/1/s/src/Languages/Shared/Editor/Services/ServiceManager.cs:line 186 at Microsoft.WebTools.Languages.Html.Editor.Document.HtmlEditorDocument..ctor(ITextBuffer textBuffer, IWebWorkspace workspace, IWebWorkspaceItem workspaceItem, IHtmlSchemaManager schemaManager, Boolean disableContainedLanguages) 
in /Users/runner/work/1/s/src/Languages/html/Editor/Document/HtmlEditorDocument.cs:line 68 at Microsoft.WebTools.Languages.Html.Editor.EditorFactory.HtmlEditorInstance..ctor(IWebWorkspace workspace, IWebWorkspaceItem workspaceItem, ITextBuffer diskBuffer, IHtmlSchemaManager schemaManager, Boolean disableContainedLanguages) 
in /Users/runner/work/1/s/src/Languages/html/Editor/EditorFactory/HtmlEditorInstance.cs:line 32 at Microsoft.WebTools.Languages.Html.Editor.EditorFactory.HtmlEditorFactory.CreateEditorInstance(IWebWorkspaceItem workspaceItem, ITextBuffer textBuffer) 
in /Users/runner/work/1/s/src/Languages/html/Editor/EditorFactory/HtmlEditorInstanceFactory.cs:line 34 at Microsoft.Ide.Web.ProjectionBufferProvider.CreateEditorInstance(IWebWorkspaceItem workspaceItem, ITextBuffer textBuffer) 
in /Users/runner/work/1/s/src/VSMac/Addin/WebTools/Html/Razor/ProjectionBufferProvider.cs:line 63 at Microsoft.Ide.Web.ProjectionBufferProvider.TryGetProjectionBuffer(ITextBuffer diskBuffer, ITextBuffer& projectionBuffer) 
in /Users/runner/work/1/s/src/VSMac/Addin/WebTools/Html/Razor/ProjectionBufferProvider.cs:line 45 at MonoDevelop.TextEditor.TextViewContent.OnGetViewControlAsync(CancellationToken token, DocumentViewContent view) 
in /Users/sandy/xam-git/vsmac/main/src/addins/MonoDevelop.TextEditor/MonoDevelop.TextEditor/TextViewContent.cs:line 177 at MonoDevelop.Ide.Gui.Documents.DocumentViewContent.LoadControl(CancellationToken cancellationToken) 
in /Users/sandy/xam-git/vsmac/main/src/core/MonoDevelop.Ide/MonoDevelop.Ide.Gui.Documents/DocumentViewContent.cs:line 151 at VisualStudioMac.UI.Shell.CocoaShellDocumentViewContent.OnLoad(CancellationToken cancellationToken) 
in /Users/sandy/xam-git/vsmac/main/src/core/VisualStudio/UI/Shell/CocoaShellDocumentViewContent.cs:line 270
--- End of inner exception stack trace ---

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@davidwengier davidwengier davidwengier approved these changes

Assignees
No one assigned
Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
2 participants
@ryanbrandenburg @davidwengier