Close over public APIs not designed for exposure #1183

rjmholt · 2020-02-10T19:26:50Z

Resolves #820.

This needs a bit of review since I'm not sure if there some other things that should be public; I've gone with a generally aggressive approach. With that said, even some of our still-public APIs aren't really designed for public consumption (like most of ScriptFile).

I've also made our code internal to the testing code so that we can test things without them needing to be public.

SeeminglyScience · 2020-02-10T19:53:55Z

I definitely use some of these in ESCS, I gotta go through and see which ones. This may take a little bit, please hold off on merging until I finish if possible.

rjmholt · 2020-02-10T20:10:41Z

please hold off on merging until I finish if possible.

Yeah I've been meaning to get to this. However, with the PowerShell 7 release deadline around the corner it would be nice for us to be able to remove these APIs before PSES 2 goes GA, rather than after.

Things like services and handlers and the dependency injection framework are things I don't think should be exposed, and really the alternate ALC was designed to hide them from PowerShell (since the issue was them polluting the default ALC).

It looks like there are a couple of cases where we have an ask for API integration and I think it makes sense for us to provide a solid (rather than incidental) public API. Given the issues with the alternate ALC, this feels like a good opportunity to:

Work out what APIs tools for PSES are using
Provide a better public API for them in the default ALC
Plumb that API through to the underlying implementation in PSES

SeeminglyScience · 2020-02-10T20:46:00Z

please hold off on merging until I finish if possible.

Yeah I've been meaning to get to this. However, with the PowerShell 7 release deadline around the corner it would be nice for us to be able to remove these APIs before PSES 2 goes GA, rather than after.

Absolutely, I agree 100%. I just want to make it clear that I'm definitely not asking for this to be closed or otherwise put on the back burner.

Things like services and handlers and the dependency injection framework are things I don't think should be exposed, and really the alternate ALC was designed to hide them from PowerShell (since the issue was them polluting the default ALC).

Well I can say for sure that the ability to directly send requests is very valuable for plugins/addons/whatever we're calling modules like ESCS. The PSES API is great and absolutely vital for simple PowerShell based editor commands. For binary modules (especially if async is utilized) though, the OmniSharp messaging API is fantastic.

On the contrast, I'd actually love to see some of this expanded. For instance, a lot of possibilities could be opened up if it was possible for an extension to set up it's own requests.

Again though, just to clarify I'm absolutely not saying that all of these API's should remain public, or even most of them.

It looks like there are a couple of cases where we have an ask for API integration and I think it makes sense for us to provide a solid (rather than incidental) public API. Given the issues with the alternate ALC, this feels like a good opportunity to:

Work out what APIs tools for PSES are using

Provide a better public API for them in the default ALC

Plumb that API through to the underlying implementation in PSES

The alternate ALC is awesome for allowing non-extension projects to run in isolation, but a supported way to load "plugins" into it may be a good idea. PSES could implement the same API's that OmniSharp already supplies, but maintaining that would be a challenge.

rjmholt · 2020-02-10T21:12:56Z

Absolutely, I agree 100%. I just want to make it clear that I'm definitely not asking for this to be closed or otherwise put on the back burner.

Thanks for being so understanding — I know you're quite impacted by this. I might be able to prioritise working on a new API that just services existing needs, which we can then work on expanding later.

The alternate ALC is awesome for allowing non-extension projects to run in isolation, but a supported way to load "plugins" into it may be a good idea. PSES could implement the same API's that OmniSharp already supplies, but maintaining that would be a challenge.

Yeah I think we need to own the whole surface of the API, or else it will clash with a dependency of some (probably Az) module, meaning we need to implement an API surface. I suspect it won't look quite like Omnisharp's, but there's some set that makes sense for us.

SeeminglyScience · 2020-02-10T21:24:31Z

Yeah I think we need to own the whole surface of the API, or else it will clash with a dependency of some (probably Az) module, meaning we need to implement an API surface. I suspect it won't look quite like Omnisharp's, but there's some set that makes sense for us.

Just to clarify, I'm not suggesting that any additional assemblies should be surfaced or that all modules should be loaded into the ALC. I'm suggesting that there should be a supported method of loading only binary modules that are meant to extend PSES into the ALC. It would be explicitly opt-in, like an assembly level attribute or something.

rjmholt · 2020-02-10T21:30:01Z

I'm suggesting that there should be a supported method of loading only binary modules that are meant to extend PSES into the ALC.

Yeah, I think of that ALC as effectively sealed though (i.e. it's where our internal implementation lives), and I'd rather expose well-defined functionality through our API rather than try and load something into that ALC. Even the module language breaks down, since a PowerShell module can only be loaded into the default ALC at present, so doing this would require such a binary module to have a part that loads into the default ALC and then call some API or ours to load itself into that ALC.

Rather than that, the PSES DLL is loaded into both, so I feel like that's what should expose an API to route into the other.

SeeminglyScience · 2020-02-10T22:00:26Z

Yeah I don't strictly disagree, and in other circumstances I'd likely be suggesting the same. My hesitation is that historically, any requests I've made or API bugs I've reported... kind of don't go anywhere (specifically the one's not likely to directly affect users). And hey look I get it, there's bigger issues and not a lot of resources. That doesn't seem super likely to really change anytime soon.

That's why the OmniSharp API is great, it's already solid, it already supports every type of request in the LSP, and it's just realistically going to get a lot more love. When I started moving things over to use the OmniSharp API was the first time in the entire time I've been working with PSES that I've thought "hey this all works and makes sense".

rjmholt · 2020-02-11T02:13:09Z

I've added an EditorEngine type, which plumbs through language server methods and has an ALC-aware service resolution method too.

The non-extension request and response types are all internal now though. Rather than expose them, we could add a hashtable-accepting request method maybe?

SeeminglyScience · 2020-02-11T03:34:52Z

@rjmholt Here's some of the OmniSharp stuff I'm talking about (sorry if this is all there already, I didn't see it at a glance):

https://github.com/SeeminglyScience/EditorServicesCommandSuite/blob/62944c0036f957723551c9fcedbc12957bf2cc29/src/EditorServicesCommandSuite.EditorServices/DocumentService.cs#L59-L96

That lets me send edits in batches. What I was doing before was:

Sending a single InsertText request for every edit
Reverse the order of edits based on offset so they don't override each other
Wait an arbitrary amount of time because there wasn't a reliable way to tell when the previous edit completed

https://github.com/SeeminglyScience/EditorServicesCommandSuite/blob/0.5.0/src/EditorServicesCommandSuite.EditorServices/WorkspaceService.cs

That whole class would be a lot harder to make reliable without the full options that the LSP gives. Additionally, while I don't currently have it set up, the LSP lets you batch file renames, deletions, moves, etc along with edits in the same "document change"

Every request, structure, etc from the LSP spec is accounted for with clean strongly typed methods

rjmholt · 2020-02-11T17:37:46Z

src/PowerShellEditorServices/Extensions/EngineExtensions/EditorEngine.cs

+
+        public EditorLanguageServer LanguageServer { get; }
+
+        public object GetService(string psesServiceFullTypeName) => GetService(psesServiceFullTypeName, "Microsoft.PowerShell.EditorServices");


So this should allow you to access arbitrary services from within the PSES ALC

Problem with this is that since the type that an external assembly would reference points to a different method table than the type returned by this method, you can't cast. This might be useful for PowerShell based extension modules, but I think the normal PSES API is suitable enough for that scenario.

(Edit: Just to clarify, casting was one of the first things I tried. I realized that kind of sounded like speculation so I just wanted to make that clear)

rjmholt · 2020-02-11T17:38:27Z

src/PowerShellEditorServices/Extensions/EngineExtensions/EditorLanguageServer.cs

+            return _languageServer.SendRequest<TResponse>(method);
+        }
+
+        public Task<TResponse> SendRequestAsync<T, TResponse>(string method, T parameters)


And this should allow you to send requests and notifications

rjmholt · 2020-02-11T17:42:46Z

One thing that won't work is exposing Omnisharp types, like the request/response types, since they're dependencies behind PSES

TylerLeonhardt · 2020-02-11T18:45:17Z

I'll review this once @SeeminglyScience's needs are met.

SeeminglyScience · 2020-02-11T19:14:18Z

One thing that won't work is exposing Omnisharp types, like the request/response types, since they're dependencies behind PSES

Isn't assembly resolution handled by a PSES delegate?

rjmholt · 2020-02-11T19:46:23Z

Isn't assembly resolution handled by a PSES delegate?

Yeah, except it's assembly resolution for the PSES ALC. Exposing types from those assemblies in a way that doesn't require reflection will just load them into the default ALC, which is what we're trying to avoid.

Rather than expose a particular dependency and pollute the PSIC session with all its types (and make it more likely that a PowerShell script will collide with or otherwise hurt the state of the PSES engine), I think the better solution is:

Use reflection to acquire needed types or methods (I tried adding a method to set the context and execute a delegate inside of it, but it didn't work)
Pull in an omnisharp DLL (or reload the one in the extension) and send objects of those types down the methods, since they just get serialised by the JSON serialiser irrespective of type

As a maintainer I'd like to make it easier than that of course, but the tools should all be there (even without the extra APIs I added), and I really do want to make sure (especially as we're about to move all this into stable) that the UX of the PSIC is as close to the gold standard (PowerShell's console host) as so many of our users seem to expect.

SeeminglyScience · 2020-02-11T20:25:39Z

Yeah, except it's assembly resolution for the PSES ALC. Exposing types from those assemblies in a way that doesn't require reflection will just load them into the default ALC, which is what we're trying to avoid.

Hmm yeah I thought this code did more

PowerShellEditorServices/src/PowerShellEditorServices.Hosting/EditorServicesLoader.cs

Lines 94 to 109 in aa00910

    
           AssemblyLoadContext.Default.Resolving += (AssemblyLoadContext defaultLoadContext, AssemblyName asmName) => 
        
           { 
        
               logger.Log(PsesLogLevel.Diagnostic, $"Assembly resolve event fired for {asmName}"); 
        
               // We only want the Editor Services DLL; the new ALC will lazily load its dependencies automatically 
        
               if (!string.Equals(asmName.Name, "Microsoft.PowerShell.EditorServices", StringComparison.Ordinal)) 
        
               { 
        
                   return null; 
        
               } 
        
               string asmPath = Path.Combine(s_psesDependencyDirPath, $"{asmName.Name}.dll"); 
        
               logger.Log(PsesLogLevel.Verbose, "Loading PSES DLL using new assembly load context"); 
        
               return psesLoadContext.LoadFromAssemblyPath(asmPath); 
        
           };

I must have been thinking of something else, my bad.

Rather than expose a particular dependency and pollute the PSIC session with all its types (and make it more likely that a PowerShell script will collide with or otherwise hurt the state of the PSES engine)

Yeah for the record I'm definitely not asking for that. Sorry if I haven't made that clear, I'm definitely looking for a solution that keeps the ALC how it is.

As a maintainer I'd like to make it easier than that of course, but the tools should all be there (even without the extra APIs I added)

Yeah, they always have been though. At the end of the day, sure I'll figure out a way to get it to work, but realistically no one else will bother. We actually have a solid, understandable API for more complex extensions, essentially for free, and I'm really uncomfortable with throwing that away.

Use reflection to acquire needed types or methods (I tried adding a method to set the context and execute a delegate inside of it, but it didn't work)

Would this be better than loading an ESCS-like assembly into PSES's ALC? If it's too hard to build that into an API then at the very least considering something like this to be within the realm of support seems like a reasonable alternative to me.

rjmholt · 2020-02-11T22:59:57Z

At the end of the day, sure I'll figure out a way to get it to work, but realistically no one else will bother

Well I'm thinking that this is in some sense by design. Given the model that involves the two ALCs right now (and only in Core), exposing the naked APIs will just make something complex look simpler than it is. The explicit hurdle to reusing our implementation (finding and using the API) is not the real one, which is the complexity of reliably and safely using our implementation bits.

We actually have a solid, understandable API for more complex extensions, essentially for free, and I'm really uncomfortable with throwing that away.

I wouldn't say we're throwing anything away. It's still there. But we haven't had time to assess what we can support. It previously being public really didn't mean we were able to support it well, even if it was things we didn't build or export ourselves. The fact of being public was not a deliberate thing in my mind. Now that the other ALC has already pushed the LSP APIs into a broken territory, we have a chance to think about and design APIs in PSES that are going to do the hard work needed for extension.

My other feeling is that the cardinality of complex plugin authors compared to it-just-works PowerShell users is such that it's better for us not to expose all this at the outset. Even in PowerShell, some slightly more complex tools use workarounds while they wait for better APIs to appear. Indeed PSES does that in multiple places. And I think that's ok for now, given that either (1) the private API won't change or (2) if it does, it will be replaced with a better public one.

Would this be better than loading an ESCS-like assembly into PSES's ALC? If it's too hard to build that into an API then at the very least considering something like this to be within the realm of support seems like a reasonable alternative to me.

I'm didn't quite follow what the difference is between your two suggestions, but I've added a method on the EditorEngine type that allows loading a DLL into the PSES ALC. I'd really prefer not to, since it can interfere with our dependency resolution if an extension loads something into it first, but I figured it will save you some time.

Hopefully that addition (and the one that gets you back the PSES ALC object itself) is enough to work with.

SeeminglyScience · 2020-02-11T23:39:57Z

Well I'm thinking that this is in some sense by design. Given the model that involves the two ALCs right now (and only in Core), exposing the naked APIs will just make something complex look simpler than it is. The explicit hurdle to reusing our implementation (finding and using the API) is not the real one, which is the complexity of reliably and safely using our implementation bits.

When I say no one else will bother, I'm talking about making complex extensions to PSES's functionality. Historically it's mostly been myself and a few paid apps have dabbled as well (though AFAIK not actually touching PSES). Sometimes it feels like making anything outside of very simple and rigid editor commands requires the same amount of scope and research as something like System.Reflection.Metadata.

I wouldn't say we're throwing anything away. It's still there. But we haven't had time to assess what we can support. It previously being public really didn't mean we were able to support it well, even if it was things we didn't build or export ourselves. The fact of being public was not a deliberate thing in my mind.

When @TylerLeonhardt was doing the OmniSharp port I specifically asked that those were made available. And FWIW the equivalent services were also purposefully made public by David back in the day (I was around when he added the original $psEditor.Components). Whether any of that was necessarily the best call is certainly up for debate, but it was definitely deliberate.

My other feeling is that the cardinality of complex plugin authors compared to it-just-works PowerShell users is such that it's better for us not to expose all this at the outset. Even in PowerShell, some slightly more complex tools use workarounds while they wait for better APIs to appear. Indeed PSES does that in multiple places. And I think that's ok for now, given that either (1) the private API won't change or (2) if it does, it will be replaced with a better public one.

Hah you don't gotta tell me that 😉. Seriously though none of this is out of concern for my own projects. I'm gonna do what I gotta do to get it to work. Hell ImpliedReflection barely uses any public API's.

I'm no stranger to it, but it's a big barrier for other folks. Especially when it's right at the gate, that can be enough to make a lot of folks dip. This is less about ESCS and more about potential growth in the PSES extension community (what is better wording for this? Extension is confusing because it sounds like I'm talking about vscode-powershell. Add-ons?). I think the OmniSharp API is a fantastic step in making that more accessible.

I'm didn't quite follow what the difference is between your two suggestions, but I've added a method on the EditorEngine type that allows loading a DLL into the PSES ALC. I'd really prefer not to, since it can interfere with our dependency resolution if an extension loads something into it first, but I figured it will save you some time.

Hopefully that addition (and the one that gets you back the PSES ALC object itself) is enough to work with.

Thanks @rjmholt !

I've pulled the PR and I'll let you know what I find!

SeeminglyScience

Got some changes, after making these public I was able to get it to work again.

I didn't personally need ICodeLensProvider or IDocumentSymbolProvider, but I know some other folks do.

SeeminglyScience · 2020-02-12T00:22:33Z

src/PowerShellEditorServices/Services/Workspace/WorkspaceService.cs

@@ -22,7 +22,7 @@ namespace Microsoft.PowerShell.EditorServices.Services
    /// Manages a "workspace" of script files that are open for a particular
    /// editing session.  Also helps to navigate references between ScriptFiles.
    /// </summary>
-    public class WorkspaceService
+    internal class WorkspaceService


I use this one a decent amount. Without it you can't get any file other than the current.

...werShellEditorServices/Services/PowerShellContext/Handlers/IInvokeExtensionCommandHandler.cs

src/PowerShellEditorServices/Services/PowerShellContext/Session/Host/PromptEvents.cs

src/PowerShellEditorServices/Services/PowerShellContext/Console/ChoiceDetails.cs

src/PowerShellEditorServices/Services/CodeLens/ICodeLensProvider.cs

src/PowerShellEditorServices/Services/Symbols/IDocumentSymbolProvider.cs

TylerLeonhardt · 2020-02-12T18:05:47Z

@SeeminglyScience do you know who these other folks are? I wanna keep a list of folks who depend on this stuff.

SeeminglyScience · 2020-02-12T18:42:01Z

@SeeminglyScience do you know who these other folks are? I wanna keep a list of folks who depend on this stuff.

For code lens/symbols? I've been asked for pointers on them a few times but I don't recall who or if any were actually completed.

I know @vexx32 was interested in a code lens for PSKoans which would be a really cool, but nothing written yet afaik.

TylerLeonhardt · 2020-02-13T18:39:36Z

@SeeminglyScience what version of ESCS works with the stable version? I wanted to show an example of the prompt message but it's not working for me with the latest version.

SeeminglyScience · 2020-02-13T19:26:05Z

@SeeminglyScience what version of ESCS works with the stable version? I wanted to show an example of the prompt message but it's not working for me with the latest version.

The one on the gallery (0.4.0?). It works with stable on Windows PowerShell without issue, on Core you need to manually remove the FileList field from the manifest, otherwise you get the "assembly with the same name already loaded" error due to a bug (or intentional change?) in pwsh (or corefx?).

rjmholt · 2020-02-13T20:41:39Z

src/PowerShellEditorServices/Extensions/Api/DocumentSymbolService.cs

+    /// <summary>
+    /// Service for registration of document symbol providers in PSES.
+    /// </summary>
+    public interface IDocumentSymbolService


I've ported this out and made it available for registration, but given that CodeLens can't come out yet (and turns out we need to change that API anyway since VSCode has changed theirs) and this is slightly hacked together, maybe it's worth disabling compilation of this file for this release?

rjmholt · 2020-02-13T20:43:14Z

src/PowerShellEditorServices/Extensions/Api/EditorContextService.cs

+    /// <summary>
+    /// Service for managing the editor context from PSES extensions.
+    /// </summary>
+    public interface IEditorContextService


This allows asynchronous file manipulation

rjmholt · 2020-02-13T20:43:39Z

src/PowerShellEditorServices/Extensions/Api/EditorExtensionServiceProvider.cs

+    /// <summary>
+    /// Object to provide extension service APIs to extensions to PSES.
+    /// </summary>
+    public class EditorExtensionServiceProvider


Here's the jumping off point for the service objects

src/PowerShellEditorServices/Extensions/Api/EditorUIService.cs

rjmholt · 2020-02-13T20:44:41Z

src/PowerShellEditorServices/Extensions/Api/ExtensionCommandService.cs

+    /// <summary>
+    /// Service for registration and invocation of extension commands.
+    /// </summary>
+    public interface IExtensionCommandService


This exposes command registration

rjmholt · 2020-02-13T20:46:23Z

src/PowerShellEditorServices/Extensions/Api/WorkspaceService.cs

+    /// <summary>
+    /// A service for querying and manipulating the editor workspace.
+    /// </summary>
+    public interface IWorkspaceService


This lets you query the workspace

rjmholt · 2020-02-13T20:47:25Z

src/PowerShellEditorServices/Extensions/EditorFileRanges.cs

+using OmniSharp.Extensions.LanguageServer.Protocol.Models;
+using System;
+
+namespace Microsoft.PowerShell.EditorServices.Extensions


This file defiles a whole set of file APIs in both 1-based and 0-based indexing conventions and can convert between them all

rjmholt · 2020-02-13T20:48:11Z

src/PowerShellEditorServices/Extensions/EditorObject.cs

+        /// <summary>
+        /// A reference to the editor object instance. Only valid when <see cref="EditorObjectReady"/> completes.
+        /// </summary>
+        public static EditorObject Instance { get; private set; }


This is so you can get the editor object instance from C#

rjmholt · 2020-02-13T20:48:27Z

src/PowerShellEditorServices/Extensions/EditorObject.cs

+        /// <summary>
+        /// A task that completes when the editor object static instance has been set.
+        /// </summary>
+        public static Task EditorObjectReady => s_editorObjectReady.Task;


This is so you can wait for the editor object to be ready

rjmholt · 2020-02-13T20:50:11Z

src/PowerShellEditorServices/Extensions/EditorObject.cs

+        /// </summary>
+        /// <param name="editorObject">The editor object ($psEditor).</param>
+        /// <returns>The extension services provider.</returns>
+        public static EditorExtensionServiceProvider GetExtensionServiceProvider(this EditorObject editorObject)


This lets you get the API from the editor object, but in a way that the method won't appear to PowerShell (for which it isn't designed)

TylerLeonhardt · 2020-02-14T19:00:19Z

@SeeminglyScience how do you feel about this PR now?

SeeminglyScience · 2020-02-14T21:01:34Z

@SeeminglyScience how do you feel about this PR now?

It's looking awesome. I still need to do some rewriting and testing in ESCS before I'll know if anything is missing though. I won't be able to do that tonight, but I'll make sure to get it done tomorrow, aiming for early morning.

rjmholt · 2020-02-14T22:24:06Z

I won't be able to do that tonight, but I'll make sure to get it done tomorrow, aiming for early morning.

No worries. We're not as rushed as we were, so take your time. Let me know if anything doesn't work or if there are more things you need and I'll be able to build it out next week.

rjmholt · 2020-02-19T22:01:05Z

@SeeminglyScience given the easier dates, we're going to do one more preview release. If the current APIs look alright to you, I'll merge this, we'll release it and then we can make any fixes/additions we need to make in the next couple weeks before this is released as GA

TylerLeonhardt

LGTM

TylerLeonhardt · 2020-02-19T22:46:11Z

double check to make sure all of the PowerShell scripts we ship have the right API calls (like psedit)

TylerLeonhardt · 2020-02-19T23:29:34Z

Here is an overview of what got changed by this pull request:

Issues
======
+ Solved 3
- Added 18
           

Complexity increasing per file
==============================
- src/PowerShellEditorServices/Extensions/EditorFileRanges.cs  4
- src/PowerShellEditorServices/Extensions/Api/ExtensionCommandService.cs  2
- src/PowerShellEditorServices/Extensions/Api/DocumentSymbolService.cs  8
- src/PowerShellEditorServices/Extensions/Api/LanguageServerService.cs  1
- src/PowerShellEditorServices/Extensions/EditorCommandAttribute.cs  1
- src/PowerShellEditorServices/Extensions/EditorObject.cs  1
- src/PowerShellEditorServices/Extensions/EditorWorkspace.cs  1
- src/PowerShellEditorServices/Extensions/Api/EditorExtensionServiceProvider.cs  2
- src/PowerShellEditorServices/Extensions/Api/EditorContextService.cs  2
- src/PowerShellEditorServices/Extensions/EditorCommand.cs  1
- src/PowerShellEditorServices/Extensions/EditorWindow.cs  1
- src/PowerShellEditorServices/Extensions/EditorRequests.cs  1
- src/PowerShellEditorServices/Extensions/EditorContext.cs  1
- src/PowerShellEditorServices/Extensions/Api/WorkspaceService.cs  2
- src/PowerShellEditorServices/Extensions/EditorTerminal.cs  1
- src/PowerShellEditorServices/Extensions/Api/EditorUIService.cs  3
- src/PowerShellEditorServices/Extensions/FileContext.cs  3
         

Complexity decreasing per file
==============================
+ test/PowerShellEditorServices.Test/Console/InputPromptHandlerTests.cs  -1
         

Clones removed
==============
+ test/PowerShellEditorServices.Test/Console/InputPromptHandlerTests.cs  -2
+ src/PowerShellEditorServices/Services/Workspace/Handlers/WorkspaceSymbolsHandler.cs  -1
+ src/PowerShellEditorServices/Services/TextDocument/Handlers/DocumentSymbolHandler.cs  -1

See the complete overview on Codacy

TylerLeonhardt · 2020-02-19T23:29:52Z

test/PowerShellEditorServices.Test/Extensions/ExtensionCommandTests.cs

+
+    /*
+    public class ExtensionCommandTests : IDisposable
+    {


Issue found: Remove this commented out code.

TylerLeonhardt · 2020-02-19T23:29:53Z

src/PowerShellEditorServices/Services/Symbols/PesterDocumentSymbolProvider.cs


-        IEnumerable<SymbolReference> IDocumentSymbolProvider.ProvideDocumentSymbols(
+        IEnumerable<ISymbolReference> IDocumentSymbolProvider.ProvideDocumentSymbols(


Issue found: Make 'ProvideDocumentSymbols' a static method.

TylerLeonhardt · 2020-02-19T23:29:54Z

src/PowerShellEditorServices/Extensions/Api/EditorExtensionServiceProvider.cs

+        {
+            _serviceProvider = serviceProvider;
+            LanguageServer = new LanguageServerService(_serviceProvider.GetService<ILanguageServer>());
+            //DocumentSymbols = new DocumentSymbolService(_serviceProvider.GetService<SymbolsService>());


Issue found: Remove this commented out code.

TylerLeonhardt · 2020-02-19T23:29:55Z

src/PowerShellEditorServices/Services/Symbols/PsdDocumentSymbolProvider.cs

-        IEnumerable<SymbolReference> IDocumentSymbolProvider.ProvideDocumentSymbols(
+        string IDocumentSymbolProvider.ProviderId => nameof(PsdDocumentSymbolProvider);
+
+        IEnumerable<ISymbolReference> IDocumentSymbolProvider.ProvideDocumentSymbols(


Issue found: Make 'ProvideDocumentSymbols' a static method.

TylerLeonhardt · 2020-02-19T23:29:57Z

src/PowerShellEditorServices/Services/Symbols/ScriptDocumentSymbolProvider.cs

    {
-        IEnumerable<SymbolReference> IDocumentSymbolProvider.ProvideDocumentSymbols(
+        string IDocumentSymbolProvider.ProviderId => nameof(ScriptDocumentSymbolProvider);


Issue found: Make 'ProviderId' a static property.

TylerLeonhardt · 2020-02-19T23:29:58Z

src/PowerShellEditorServices/Extensions/Api/EditorExtensionServiceProvider.cs

+        /// In .NET Framework, this returns null.
+        /// </summary>
+        /// <returns>The assembly load context used for loading PSES, or null in .NET Framework.</returns>
+        public object GetPsesAssemblyLoadContext()


Issue found: Make 'GetPsesAssemblyLoadContext' a static method.

TylerLeonhardt · 2020-02-19T23:30:00Z

src/PowerShellEditorServices/Services/Symbols/ScriptDocumentSymbolProvider.cs

    {
-        IEnumerable<SymbolReference> IDocumentSymbolProvider.ProvideDocumentSymbols(
+        string IDocumentSymbolProvider.ProviderId => nameof(ScriptDocumentSymbolProvider);


Issue found: Remove the unused private property 'IDocumentSymbolProvider.ProviderId'.

TylerLeonhardt · 2020-02-19T23:30:01Z

src/PowerShellEditorServices/Extensions/Api/ExtensionCommandService.cs

+            CommandUpdated?.Invoke(this, editorCommand);
+        }
+
+        private void OnCommandRemoved(object sender, EditorCommand editorCommand)


Issue found: Remove this unused method parameter 'sender'.

TylerLeonhardt · 2020-02-19T23:30:03Z

src/PowerShellEditorServices/Extensions/Api/EditorExtensionServiceProvider.cs

+        /// </summary>
+        /// <param name="assemblyPath">The absolute path of the assembly to load.</param>
+        /// <returns>The loaded assembly object.</returns>
+        public Assembly LoadAssemblyInPsesLoadContext(string assemblyPath)


Issue found: Make 'LoadAssemblyInPsesLoadContext' a static method.

TylerLeonhardt · 2020-02-19T23:30:04Z

src/PowerShellEditorServices/Extensions/Api/EditorExtensionServiceProvider.cs

+        /// <summary>
+        /// Service providing document symbol provider registration.
+        /// </summary>
+        // public IDocumentSymbolService DocumentSymbols { get; }


Issue found: Remove this commented out code.

TylerLeonhardt · 2020-02-19T23:30:05Z

src/PowerShellEditorServices/Extensions/Api/ExtensionCommandService.cs

+            CommandAdded?.Invoke(this, editorCommand);
+        }
+
+        private void OnCommandUpdated(object sender, EditorCommand editorCommand)


Issue found: Remove this unused method parameter 'sender'.

TylerLeonhardt · 2020-02-19T23:30:06Z

src/PowerShellEditorServices/Services/Symbols/PesterDocumentSymbolProvider.cs

    {
+        string IDocumentSymbolProvider.ProviderId => nameof(PesterDocumentSymbolProvider);


Issue found: Remove the unused private property 'IDocumentSymbolProvider.ProviderId'.

TylerLeonhardt · 2020-02-19T23:30:08Z

test/PowerShellEditorServices.Test/Extensions/ExtensionCommandTests.cs

+using System.Threading.Tasks;
+using Xunit;
+
+namespace Microsoft.PowerShell.EditorServices.Test.Extensions


Issue found: Remove this empty namespace.

TylerLeonhardt · 2020-02-19T23:30:09Z

src/PowerShellEditorServices/Extensions/Api/ExtensionCommandService.cs

+
+        public void UnregisterCommand(string commandName) => _extensionService.UnregisterCommand(commandName);
+
+        private void OnCommandAdded(object sender, EditorCommand editorCommand)


Issue found: Remove this unused method parameter 'sender'.

TylerLeonhardt · 2020-02-19T23:30:10Z

src/PowerShellEditorServices/Services/Symbols/PsdDocumentSymbolProvider.cs

    {
-        IEnumerable<SymbolReference> IDocumentSymbolProvider.ProvideDocumentSymbols(
+        string IDocumentSymbolProvider.ProviderId => nameof(PsdDocumentSymbolProvider);


Issue found: Remove the unused private property 'IDocumentSymbolProvider.ProviderId'.

TylerLeonhardt · 2020-02-19T23:30:11Z

src/PowerShellEditorServices/Extensions/Api/WorkspaceService.cs

+            return files.AsReadOnly();
+        }
+
+        private IEditorScriptFile GetEditorFileFromScriptFile(ScriptFile file)


Issue found: Make 'GetEditorFileFromScriptFile' a static method.

TylerLeonhardt · 2020-02-19T23:30:12Z

src/PowerShellEditorServices/Services/Symbols/PesterDocumentSymbolProvider.cs

    {
+        string IDocumentSymbolProvider.ProviderId => nameof(PesterDocumentSymbolProvider);


Issue found: Make 'ProviderId' a static property.

TylerLeonhardt · 2020-02-19T23:30:13Z

src/PowerShellEditorServices/Services/Symbols/ScriptDocumentSymbolProvider.cs

-        IEnumerable<SymbolReference> IDocumentSymbolProvider.ProvideDocumentSymbols(
+        string IDocumentSymbolProvider.ProviderId => nameof(ScriptDocumentSymbolProvider);
+
+        IEnumerable<ISymbolReference> IDocumentSymbolProvider.ProvideDocumentSymbols(


Issue found: Make 'ProvideDocumentSymbols' a static method.

rjmholt · 2020-02-19T23:36:53Z

I'm going to merge this so I can get going on our build process, but just let me know if there are issues and we can fix them

rjmholt requested review from TylerLeonhardt, rkeithhill and SeeminglyScience February 10, 2020 19:26

rjmholt commented Feb 11, 2020

View reviewed changes

SeeminglyScience requested changes Feb 12, 2020

View reviewed changes

PowerShell deleted a comment from rjmholt Feb 13, 2020

rjmholt commented Feb 13, 2020

View reviewed changes

Rob Holt added 4 commits February 18, 2020 14:31

Add skeleton of tests

34e9407

Increase delay

0242568

Get rid of bad tests

e3f4b71

Disable bad hotkey test

acacde4

SeeminglyScience mentioned this pull request Feb 19, 2020

ConvertTo-SplatExpression: Cannot find type for custom attribute 'EditorCommand' SeeminglyScience/EditorServicesCommandSuite#42

Closed

TylerLeonhardt approved these changes Feb 19, 2020

View reviewed changes

Rename UI service APIs to have Async suffix

7fccc46

TylerLeonhardt reviewed Feb 19, 2020

View reviewed changes

rjmholt merged commit 945d616 into PowerShell:master Feb 19, 2020


		public EditorLanguageServer LanguageServer { get; }

		public object GetService(string psesServiceFullTypeName) => GetService(psesServiceFullTypeName, "Microsoft.PowerShell.EditorServices");


		IEnumerable<SymbolReference> IDocumentSymbolProvider.ProvideDocumentSymbols(
		IEnumerable<ISymbolReference> IDocumentSymbolProvider.ProvideDocumentSymbols(

		{
		string IDocumentSymbolProvider.ProviderId => nameof(PesterDocumentSymbolProvider);


		public void UnregisterCommand(string commandName) => _extensionService.UnregisterCommand(commandName);

		private void OnCommandAdded(object sender, EditorCommand editorCommand)

Close over public APIs not designed for exposure #1183

Close over public APIs not designed for exposure #1183

Conversation

rjmholt commented Feb 10, 2020

SeeminglyScience commented Feb 10, 2020

rjmholt commented Feb 10, 2020

SeeminglyScience commented Feb 10, 2020

rjmholt commented Feb 10, 2020

SeeminglyScience commented Feb 10, 2020

rjmholt commented Feb 10, 2020

SeeminglyScience commented Feb 10, 2020

rjmholt commented Feb 11, 2020

SeeminglyScience commented Feb 11, 2020

Choose a reason for hiding this comment

SeeminglyScience Feb 11, 2020 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

rjmholt commented Feb 11, 2020

TylerLeonhardt commented Feb 11, 2020

SeeminglyScience commented Feb 11, 2020

rjmholt commented Feb 11, 2020

SeeminglyScience commented Feb 11, 2020

rjmholt commented Feb 11, 2020

SeeminglyScience commented Feb 11, 2020

SeeminglyScience left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

TylerLeonhardt commented Feb 12, 2020

SeeminglyScience commented Feb 12, 2020

TylerLeonhardt commented Feb 13, 2020

SeeminglyScience commented Feb 13, 2020

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

TylerLeonhardt commented Feb 14, 2020

SeeminglyScience commented Feb 14, 2020

rjmholt commented Feb 14, 2020

rjmholt commented Feb 19, 2020

TylerLeonhardt left a comment

Choose a reason for hiding this comment

TylerLeonhardt commented Feb 19, 2020

TylerLeonhardt commented Feb 19, 2020

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

rjmholt commented Feb 19, 2020

SeeminglyScience Feb 11, 2020 •

edited

Loading