Skip to content

Remove all pipelines #690

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.

Sign up for GitHub

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

Closed
wants to merge 4 commits into from
Closed

Remove all pipelines #690

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
4 commits
Select commit Hold shift + click to select a range
3e0cb94
remove all references to the Pipeline class
TylerLeonhardt Jun 23, 2018
b20021f
patrick's feedback
TylerLeonhardt Jun 24, 2018
bfe3d43
debugging
TylerLeonhardt Jun 24, 2018
d8d1e51
address patrick's feedback
Jul 2, 2018
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
1 change: 0 additions & 1 deletion PowerShellEditorServices.Common.props
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -9,6 +9,5 @@
<RepositoryType>git</RepositoryType>
<RepositoryUrl>https://github.com/PowerShell/PowerShellEditorServices</RepositoryUrl>
<DebugType>portable</DebugType>
<RuntimeFrameworkVersion>1.0.3</RuntimeFrameworkVersion>
</PropertyGroup>
</Project>
27 changes: 6 additions & 21 deletions src/PowerShellEditorServices/Session/PowerShell3Operations.cs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -6,11 +6,12 @@
using System;
using System.Collections.Generic;
using System.Linq;
using System.Management.Automation;
using System.Management.Automation.Runspaces;

namespace Microsoft.PowerShell.EditorServices.Session
{
using System.Management.Automation;
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

There's a problem with putting this using statement outside the namespace isn't there? We should leave a comment as to why.

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

not sure why yet... :(


internal class PowerShell3Operations : IVersionSpecificOperations
{
public void ConfigureDebugger(Runspace runspace)
Expand All @@ -32,27 +33,11 @@ public IEnumerable<TResult> ExecuteCommandInDebugger<TResult>(
out DebuggerResumeAction? debuggerResumeAction)
{
IEnumerable<TResult> executionResult = null;

using (var nestedPipeline = currentRunspace.CreateNestedPipeline())
using (var pwsh = PowerShell.Create())
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I'd use the type name rather than var here, since it's not apparent from Create().

Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I think static Create factory methods are common enough (especially in the PS engine) that they might as well be the same syntax as new PowerShell(). I don't feel particularly strongly about it or anything, just weighing in.

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I left it as is this time since it's the pattern we currently use everywhere.

{
foreach (var command in psCommand.Commands)
{
nestedPipeline.Commands.Add(command);
}

var results = nestedPipeline.Invoke();

if (typeof(TResult) != typeof(PSObject))
{
executionResult =
results
.Select(pso => pso.BaseObject)
.Cast<TResult>();
}
else
{
executionResult = results.Cast<TResult>();
}
pwsh.Runspace = currentRunspace;
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I seem to recall seeing somewhere that setting Runspace here to currentRunspace does something special. It might be worth a comment about that if so.

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

What special does it do? :)

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Like it replaces nested pipelines or something? Maybe I'm remembering wrong...

Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I believe your thinking of PowerShell.Create(RunspaceMode.CurrentRunspace). In this case it's just a field, nothing special.

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Ok perfect!

pwsh.Commands = psCommand;
executionResult = pwsh.Invoke<TResult>();
}

// Write the output to the host if necessary
Expand Down
65 changes: 10 additions & 55 deletions src/PowerShellEditorServices/Session/PowerShellContext.cs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -823,44 +823,11 @@ await this.ExecuteCommand<object>(

internal static TResult ExecuteScriptAndGetItem<TResult>(string scriptToExecute, Runspace runspace, TResult defaultValue = default(TResult))
{
Pipeline pipeline = null;

try
using (var pwsh = PowerShell.Create())
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Same comment on var

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I left it as is this time since it's the pattern we currently use everywhere.

{
if (runspace.RunspaceAvailability == RunspaceAvailability.AvailableForNestedCommand)
{
pipeline = runspace.CreateNestedPipeline(scriptToExecute, false);
}
else
{
pipeline = runspace.CreatePipeline(scriptToExecute, false);
}

Collection<PSObject> results = pipeline.Invoke();

if (results.Count == 0 || results.FirstOrDefault() == null)
{
return defaultValue;
}

if (typeof(TResult) != typeof(PSObject))
{
return results
.Select(pso => pso.BaseObject)
.OfType<TResult>()
.FirstOrDefault();
}
else
{
return
results
.OfType<TResult>()
.FirstOrDefault();
}
}
finally
{
pipeline.Dispose();
pwsh.Runspace = runspace;
Collection<TResult> results = pwsh.AddScript(scriptToExecute).Invoke<TResult>();
return results.DefaultIfEmpty(defaultValue).First();
}
}

Expand Down Expand Up @@ -1526,6 +1493,12 @@ private async Task<SessionDetails> GetSessionDetailsInRunspace()
}
}

private SessionDetails GetSessionDetailsInNestedPipeline()
{

return GetSessionDetailsInRunspace(this.CurrentRunspace.Runspace);
}

private SessionDetails GetSessionDetailsInRunspace(Runspace runspace)
{
SessionDetails sessionDetails =
Expand Down Expand Up @@ -1561,24 +1534,6 @@ private SessionDetails GetSessionDetailsInDebugger()
});
}

private SessionDetails GetSessionDetailsInNestedPipeline()
{
using (var pipeline = this.CurrentRunspace.Runspace.CreateNestedPipeline())
{
return this.GetSessionDetails(
command =>
{
pipeline.Commands.Clear();
pipeline.Commands.Add(command.Commands[0]);

return
pipeline
.Invoke()
.FirstOrDefault();
});
}
}

private void SetProfileVariableInCurrentRunspace(ProfilePaths profilePaths)
{
// Create the $profile variable
Expand Down