Markup Microsoft.PowerShell.EditorServices.Hosting

Author: andyleejordan

src/PowerShellEditorServices/Hosting/EditorServicesLoading.cs

@@ -11,7 +11,7 @@ internal static class EditorServicesLoading
     {
         internal static void LoadEditorServicesForHost()
         {
-            // No op that forces loading this assembly
+            // No-op that forces loading this assembly
         }
     }
 }

src/PowerShellEditorServices/Hosting/EditorServicesServerFactory.cs

@@ -21,15 +21,22 @@ namespace Microsoft.PowerShell.EditorServices.Hosting
 {
     /// <summary>
     /// Factory class for hiding dependencies of Editor Services.
-    /// In particular, dependency injection and logging are wrapped by factory methods on this class
-    /// so that the host assembly can construct the LSP and debug servers
-    /// without taking logging or dependency injection dependencies directly.
+    ///
+    /// NOTE: dependency injection and logging are wrapped by factory methods on this class so that
+    /// the host assembly can construct the LSP and debug servers without directly depending on <see
+    /// cref="Microsoft.Extensions.Logging"/>
+    /// and <see cref="Microsoft.Extensions.DependencyInjection"/>.
     /// </summary>
     internal class EditorServicesServerFactory : IDisposable
     {
         /// <summary>
-        /// Create a new Editor Services factory.
-        /// This method will instantiate logging.
+        /// Create a new Editor Services factory. This method will instantiate logging.
+        ///
+        /// NOTE: This can only be called once because it sets global state (the logger) and that
+        /// call is in <see cref="EditorServicesRunner"/>.
+        ///
+        /// TODO: Why is this a static function wrapping a constructor instead of just a
+        /// constructor? In the end it returns an instance (albeit a "singleton").
         /// </summary>
         /// <param name="logPath">The path of the log file to use.</param>
         /// <param name="minimumLogLevel">The minimum log level to use.</param>
@@ -69,10 +76,15 @@ private EditorServicesServerFactory(ILoggerFactory loggerFactory, LogLevel minim
 
         /// <summary>
         /// Create the LSP server.
+        ///
+        /// NOTE: This is only called once and that's in <see cref="EditorServicesRunner"/>.
+        ///
+        /// TODO: Why is this a method on this class? It only uses <see cref="_loggerFactory"/>.
         /// </summary>
         /// <param name="inputStream">The protocol transport input stream.</param>
         /// <param name="outputStream">The protocol transport output stream.</param>
-        /// <param name="hostStartupInfo">The host details configuration for Editor Services instantation.</param>
+        /// <param name="hostStartupInfo">The host details configuration for Editor Services
+        /// instantation.</param>
         /// <returns>A new, unstarted language server instance.</returns>
         public PsesLanguageServer CreateLanguageServer(
             Stream inputStream,
@@ -84,42 +96,73 @@ public PsesLanguageServer CreateLanguageServer(
 
         /// <summary>
         /// Create the debug server given a language server instance.
+        ///
+        /// NOTE: This is only called once and that's in <see cref="EditorServicesRunner"/>.
+        ///
+        /// TODO: Why is this a method on this class? It only uses <see cref="_loggerFactory"/>.
         /// </summary>
         /// <param name="inputStream">The protocol transport input stream.</param>
         /// <param name="outputStream">The protocol transport output stream.</param>
         /// <param name="languageServer"></param>
         /// <returns>A new, unstarted debug server instance.</returns>
-        public PsesDebugServer CreateDebugServerWithLanguageServer(Stream inputStream, Stream outputStream, PsesLanguageServer languageServer, bool usePSReadLine)
+        public PsesDebugServer CreateDebugServerWithLanguageServer(
+            Stream inputStream,
+            Stream outputStream,
+            PsesLanguageServer languageServer,
+            bool usePSReadLine)
         {
-            return new PsesDebugServer(_loggerFactory, inputStream, outputStream, languageServer.LanguageServer.Services, useTempSession: false, usePSReadLine);
+            return new PsesDebugServer(
+                _loggerFactory,
+                inputStream,
+                outputStream,
+                languageServer.LanguageServer.Services,
+                useTempSession: false,
+                usePSReadLine);
         }
 
         /// <summary>
         /// Create a new debug server based on an old one in an ended session.
+        ///
+        /// NOTE: This is only called once and that's in <see cref="EditorServicesRunner"/>.
+        ///
+        /// TODO: Why is this a method on this class? It only uses <see cref="_loggerFactory"/>.
         /// </summary>
         /// <param name="inputStream">The protocol transport input stream.</param>
         /// <param name="outputStream">The protocol transport output stream.</param>
         /// <param name="debugServer">The old debug server to recreate.</param>
         /// <returns></returns>
-        public PsesDebugServer RecreateDebugServer(Stream inputStream, Stream outputStream, PsesDebugServer debugServer, bool usePSReadLine)
+        public PsesDebugServer RecreateDebugServer(
+            Stream inputStream,
+            Stream outputStream,
+            PsesDebugServer debugServer,
+            bool usePSReadLine)
         {
-            return new PsesDebugServer(_loggerFactory, inputStream, outputStream, debugServer.ServiceProvider, useTempSession: false, usePSReadLine);
+            return new PsesDebugServer(
+                _loggerFactory,
+                inputStream,
+                outputStream,
+                debugServer.ServiceProvider,
+                useTempSession: false,
+                usePSReadLine);
         }
 
         /// <summary>
         /// Create a standalone debug server for temp sessions.
+        ///
+        /// TODO: What is a "temp session" and how is this different?
         /// </summary>
         /// <param name="inputStream">The protocol transport input stream.</param>
         /// <param name="outputStream">The protocol transport output stream.</param>
         /// <param name="hostStartupInfo">The host startup configuration to create the server session with.</param>
         /// <returns></returns>
-        public PsesDebugServer CreateDebugServerForTempSession(Stream inputStream, Stream outputStream, HostStartupInfo hostStartupInfo)
+        public PsesDebugServer CreateDebugServerForTempSession(
+            Stream inputStream, Stream outputStream, HostStartupInfo hostStartupInfo)
         {
             var serviceProvider = new ServiceCollection()
                 .AddLogging(builder => builder
                     .ClearProviders()
                     .AddSerilog()
-                    .SetMinimumLevel(LogLevel.Trace))
+                    .SetMinimumLevel(LogLevel.Trace)) // TODO: Why randomly set to trace?
                 .AddSingleton<ILanguageServerFacade>(provider => null)
                 .AddPsesLanguageServices(hostStartupInfo)
                 // For a Temp session, there is no LanguageServer so just set it to null
@@ -143,6 +186,14 @@ public PsesDebugServer CreateDebugServerForTempSession(Stream inputStream, Strea
                 usePSReadLine: hostStartupInfo.ConsoleReplEnabled && !hostStartupInfo.UsesLegacyReadLine);
         }
 
+        /// <summary>
+        /// TODO: This class probably should not be <see cref="IDisposable"/> as the primary
+        /// intention of that interface is to provide cleanup of unmanaged resources, which the
+        /// logger certainly is not. Nor is this class used with a <see langword="using"/>. Instead,
+        /// this class should call <see cref="Serilog.Log.CloseAndFlush()"/> in a finalizer. This
+        /// could potentially even be done with <see
+        /// cref="SerilogLoggerFactoryExtensions.AddSerilog"</> by passing <c>dispose=true</c>.
+        /// </summary>
         public void Dispose()
         {
             Log.CloseAndFlush();

src/PowerShellEditorServices/Hosting/HostStartupInfo.cs

@@ -9,8 +9,10 @@
 namespace Microsoft.PowerShell.EditorServices.Hosting
 {
     /// <summary>
-    /// Contains details about the host as well as any other information
-    /// needed by Editor Services at startup time.
+    /// Contains details about the host as well as any other information needed by Editor Services
+    /// at startup time.
+    ///
+    /// TODO: Can this be simplified as a <see langword="record"/>?
     /// </summary>
     public sealed class HostStartupInfo
     {
@@ -96,6 +98,10 @@ public sealed class HostStartupInfo
 
         /// <summary>
         /// The minimum log level of log events to be logged.
+        ///
+        /// NOTE: This is cast to all of <see cref="PsesLogLevel"/>, <see
+        /// cref="Microsoft.Extensions.Logging.LogLevel"/>, and <see
+        /// cref="Serilog.Events.LogEventLevel"/>, hence it is an <c>int</c>.
         /// </summary>
         public int LogLevel { get; }
 
@@ -158,6 +164,12 @@ public HostStartupInfo(
         #endregion
     }
 
+    /// <summary>
+    /// This is a strange class that is generally <c>null</c> or otherwise just has a single path
+    /// set. It is eventually parsed one-by-one when setting up the PowerShell runspace.
+    ///
+    /// TODO: This could probably be very simplified.
+    /// </summary>
     public sealed class ProfilePathInfo
     {
         public ProfilePathInfo(

src/PowerShellEditorServices/Logging/PsesTelemetryEvent.cs

@@ -6,7 +6,7 @@
 
 namespace Microsoft.PowerShell.EditorServices.Logging
 {
-    // This inheirits from Dictionary so that it can be passed in to SendTelemetryEvent()
+    // This inherits from Dictionary so that it can be passed in to SendTelemetryEvent()
     // which takes in an IDictionary<string, object>
     // However, I wanted creation to be easy so you can do
     // new PsesTelemetryEvent { EventName = "eventName", Data = data }