Markup PowerShellEditorServices/Hosting

As opposed to `PowerShellEditorServices.Hosting`. Same namespace, different assembly.

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,16 +21,25 @@ 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.
     /// </summary>
+    /// <remarks>
+    /// 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"/>.
+    /// </remarks>
     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.
         /// </summary>
+        /// <remarks>
+        /// 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").
+        /// </remarks>
         /// <param name="logPath">The path of the log file to use.</param>
         /// <param name="minimumLogLevel">The minimum log level to use.</param>
         /// <returns></returns>
@@ -54,6 +63,9 @@ public static EditorServicesServerFactory Create(string logPath, int minimumLogL
             return new EditorServicesServerFactory(loggerFactory, (LogLevel)minimumLogLevel);
         }
 
+        // TODO: Can we somehow refactor this member so the language and debug servers can be
+        // instantiated using their constructors instead of tying them to this factory with `Create`
+        // methods?
         private readonly ILoggerFactory _loggerFactory;
 
         private readonly Microsoft.Extensions.Logging.ILogger _logger;
@@ -70,9 +82,13 @@ private EditorServicesServerFactory(ILoggerFactory loggerFactory, LogLevel minim
         /// <summary>
         /// Create the LSP server.
         /// </summary>
+        /// <remarks>
+        /// This is only called once and that's in <see cref="EditorServicesRunner"/>.
+        /// </remarks>
         /// <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,
@@ -85,25 +101,51 @@ public PsesLanguageServer CreateLanguageServer(
         /// <summary>
         /// Create the debug server given a language server instance.
         /// </summary>
+        /// <remarks>
+        /// This is only called once and that's in <see cref="EditorServicesRunner"/>.
+        /// </remarks>
         /// <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.
         /// </summary>
+        /// <remarks>
+        /// This is only called once and that's in <see cref="EditorServicesRunner"/>.
+        /// </remarks>
         /// <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>
@@ -113,13 +155,16 @@ public PsesDebugServer RecreateDebugServer(Stream inputStream, Stream outputStre
         /// <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 +188,14 @@ public PsesDebugServer CreateDebugServerForTempSession(Stream inputStream, Strea
                 usePSReadLine: hostStartupInfo.ConsoleReplEnabled && !hostStartupInfo.UsesLegacyReadLine);
         }
 
+        /// <remarks>
+        /// 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>.
+        /// </remarks>
         public void Dispose()
         {
             Log.CloseAndFlush();

src/PowerShellEditorServices/Hosting/HostStartupInfo.cs

@@ -9,9 +9,12 @@
 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.
     /// </summary>
+    /// <remarks>
+    /// TODO: Simplify this as a <see langword="record"/>.
+    /// </remarks>
     public sealed class HostStartupInfo
     {
         #region Constants
@@ -97,6 +100,11 @@ public sealed class HostStartupInfo
         /// <summary>
         /// The minimum log level of log events to be logged.
         /// </summary>
+        /// <remarks>
+        /// 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>.
+        /// </remarks>
         public int LogLevel { get; }
 
         #endregion
@@ -158,6 +166,13 @@ 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.
+    /// </summary>
+    /// <remarks>
+    /// TODO: Simplify this as a <see langword="record"/>.
+    /// </remarks>
     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 }