-
Notifications
You must be signed in to change notification settings - Fork 210
Support cohosting in VS Code #11748
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
Support cohosting in VS Code #11748
Conversation
We're going to reference a shared project, so having it match the rest of our repo makes things a lot easier
…reference from VS and VS Code layers
099becb
to
063901b
Compare
Goes with dotnet/vscode-csharp#8189 and dotnet/razor#11748 to allow Razor cohosting in VS Code, but also fixes some VSIX issues that broke cohosting in VS too. Reviewing commit-at-a-time is probably recommended.
@dotnet/razor-tooling PTAL |
|
||
namespace Microsoft.VisualStudioCode.RazorExtension.Services; | ||
|
||
// TODO: |
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.
nit: file a follow up
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.
It's a bullet point under 9 on #9519 :)
src/Razor/src/Microsoft.VisualStudioCode.RazorExtension/Services/LspLogger.cs
Outdated
Show resolved
Hide resolved
@@ -1,8 +1,13 @@ | |||
// Copyright (c) .NET Foundation. All rights reserved. | |||
// Licensed under the MIT license. See License.txt in the project root for license information. | |||
|
|||
using System; |
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.
change to implicit usings?
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.
I don't know how to do emojies on mac so just imagine a questionmark emoji so that this is inquisitive and not a suggestion or aggrevation.
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.
Can't really. The 2nd commit in this PR explicitly turns off implicit usings in this project, because now that it references the shared project, it has to be in lock step with the other thing that references it. ie, could do this, but have to do it in MS.VS.LanguageServices.Razor too. So if we feel strongly enough, not this PR :)
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.
No strong opinion, just noted a change that looks weird and was hoping turning them off was why
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.
Oh, and press the globe looking button in the bottom left of the keyboard :)
using Microsoft.VisualStudio.Threading; | ||
|
||
namespace Microsoft.VisualStudio.Razor.Settings; | ||
|
||
[Export(typeof(IClientSettingsReader))] |
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.
I'm confused that this actually works. I see that IClientSettingsReader has GetClientSettings()
which this also has. Is that enough for MEF? It doesn't actually have to have the IClientSettingsReader
in it's type ancestry?
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.
I made IClientSettingsManager
implement IClientSettingsReader
just so I didn't have to go and update everywhere that used it, so yes, it is in its type ancestry.
Co-authored-by: Andrew Hall <[email protected]>
Goes with dotnet/roslyn#78167 and dotnet/vscode-csharp#8189 to expose cohosting to VS Code.
Needs dotnet/roslyn#78167 merged and referenced before it will build.
Part of #11759