Is this TODO comment valid anymore?

I think the commend can be removed

We do the same thing in the else block so we don't need this condition.

Yes, will move it out

Nitpick. I think this is easier to read if the condition expression is in parentheses: ... = (value != null) ? ...

I can fix that

It looks like this constructor is no longer used (and the below ISS one is now only used). AuthorizationManager property is not assigned here and the code looks like it assumes it is never null, so I think we should remove this constructor.

I'll check; if this isn't used anymore I'll remove it

I don't understand why this was removed. It looks like the only purpose of this helper method is to write to file.

PSConsoleFile is not supported as it was only used with RunspaceConfiguration

But SaveAsConsoleFile() now no longer does anything useful. So I think this entire method should be removed.

Yes, SaveAsConsoleFile() should be removed

I don't understand why this was removed. It looks like the only purpose of this helper method is to write to file.

PSConsoleFile is not supported as it was only used with RunspaceConfiguration

But SaveAsConsoleFile() now no longer does anything useful. So I think this entire method should be removed.

Yes, SaveAsConsoleFile() should be removed

This may not be the same as creating a runspace with default RunspaceConfiguration and so the behavior may not be the same as before. [runspacefactory]::CreateRunspace($host). I don't know if this would make a noticeable difference but wanted to point it out.

Yes, this is a breaking change. Our test coverage may not be sufficient to cover this so I need someone with experience here to point out how this may have adverse effects.

If we want to mimic the existing behavior, then we should use InitialSessionState.CreateDefault() here. When using RunspaceConfiguration all built-in powershell modules are loaded as snapins, so you will get nothing when running Get-Module even after executing Get-Process. With CreateDefault(), built-in powershell modules are loaded as snapins as well. But with CreateDefault2(), only the Microsoft.PowerShell.Core is loaded as snapin and the rest are loaded as modules, like a regular powershell console session does.

Declaring and assigning result can now be done on one line. No need to have separate lines. Also can _initialSessionState be null here? The modified constructor above looks like it constructs this object without rsConfig and _initialSessionState is null.

Can this file just be removed? It looks like it only contains code related to runspace configuration.

I think I originally wanted to remove it, but it contains an enum used by FormatAndTypeDataHelper which I think is still needed for some PSSnapins we ship even if we don't support custom PSSnapins

You could rename - it's being used in Update-FormatData which is not snap-in related.

Moving enum to FormatAndTypeDataHelper.cs and moving that file to utils. Removing the last of the RunspaceConfiguration*.cs files

I take that back, RunspaceConfigurationEntry.cs is still needed. I'll move the enum to this file put it under engine. We can get rid of the minishell folder.

This looks strange. Can you remove the bracket from the comment?

Yes, that was probably just a mistake

Are we sure that we want to use CreateDefault() and load all commands/functions? Or CreateDefault2() where just Core commands are loaded (and is faster)? Please add a comment to justify which default we use.

This is where I need some expertise to mimic the previous RunspaceConfiguration behavior. @lzybkr @daxian-dbw ?

I'm not sure this api makes sense anymore - it was accepting a runspace config, so the replacement would take an initial session state, but those apis already exist.

In reply to: 144347463 [](ancestors = 144347463)

It looks to me it's always SingleShell now, as InitialSessionState is initialized in the constructor and cannot be null.
So, all IsSingleShell related the code can be cleaned as well.

yup, will clean up

I'm not sure we should remove this method, but this extra loop is pointless - just copying the keys to a temp collection.

I wasn't sure about this method. I set a breakpoint in it and couldn't get it to hit within the if statement using runspace creation. It also seems that RemoveProvider() is only used by RunspaceConfiguration

The if statement looks to me is checking if the current EngineSessionState is a module session state and if the module defines providers. This seems will happen when a Runspace is closing while running cmdlets from a module.

RemoveProvider(string providerName, bool force, CmdletProviderContext context) doesn't look like solely related to RunspaceConfigration.

I think we should keep this method but remove the extra loop.

Same question here. This method doesn't look like solely related to RunspaceConfiguration.

Putting back

These following two methods seem OK to be removed:

private void RemoveProvider(ProviderConfigurationEntry entry)
private string GetProviderName(ProviderConfigurationEntry entry)

Will remove.

This change seems unnecessary. The extra space should be removed.

will fix

This may not be the same as creating a runspace with default RunspaceConfiguration and so the behavior may not be the same as before. [runspacefactory]::CreateRunspace($host). I don't know if this would make a noticeable difference but wanted to point it out.

Yes, this is a breaking change. Our test coverage may not be sufficient to cover this so I need someone with experience here to point out how this may have adverse effects.

If we want to mimic the existing behavior, then we should use InitialSessionState.CreateDefault() here. When using RunspaceConfiguration all built-in powershell modules are loaded as snapins, so you will get nothing when running Get-Module even after executing Get-Process. With CreateDefault(), built-in powershell modules are loaded as snapins as well. But with CreateDefault2(), only the Microsoft.PowerShell.Core is loaded as snapin and the rest are loaded as modules, like a regular powershell console session does.

Same here, if we want to mimic the existing behavior, then we should use CreateDefault.

will change

Sorry if you already explained this change in some previous conversations, but I didn't find any.
The original code seems already makes sure that StopAllTranscribing will be called only if all runspaces are at closing state. As for AmsiUtils.Uninitialize(), it looks to me doesn't need to be called for every Runspace close operation, but stead should only be called once when the last Runspace is closed.

@lzybkr had a question about this section in that previously it would return out of this method early
if it's not the last runspace so that transcription would not be stopped. Because it returned early, the rest of the closing actions would not be executed.

The rest of the closing method is AmsiUtils.Uninitialize(); which I think should only execute once when the last Runspace is closing. It looks to me AmsiUtils.Init() should run only once and then can be shared across all Runspaces.

I remember AMSI is thread safe and @PaulHigin even fixed a bug previously to remove the locks when calling AmsiUtils.ScanContent. So I think it should be initialized once and used across all Runspaces.

I'll change it so if there's no open runspaces left, transcription is stopped and AmsiUtils.Uninitialize() is called.

Same here. If we want to mimic RunspaceConfiguration, then CreateDefault() should be used.

will change

I think this method should be removed, and update the two call sites in HelpProviderWithFullCache.cs:
if (!this.CacheFullyLoaded || AreSnapInsSupported()) ==> if (!this.CacheFullyLoaded)

will update and remove no longer relevant comment

How about change it to the following to remove 2 duplicate lines of InitialSessionState.CreateDefault2()?

#if DEBUG
            if (args.Length > 0 && !String.IsNullOrEmpty(args[0]) && args[0].Equals("-isswait", StringComparison.OrdinalIgnoreCase))
            {
                Console.WriteLine("Attach the debugger to continue...");
                while (!System.Diagnostics.Debugger.IsAttached) {
                    Thread.Sleep(100);
                }
                System.Diagnostics.Debugger.Break();
            }
#endif
            ConsoleHost.DefaultInitialSessionState = InitialSessionState.CreateDefault2();

sounds good

AmsiUtils.Uninitialize() should be called even if host == null.
It looks to me we don't need to change this method. Here is the logic of the existing method:

• if there is still an open Runsapce, then this method will just return;
• if there is no open Runspace, then the method will continue: call StopAllTranscribing if host != null, and then call AmsiUtils.Uninitialize().

will revert previous change