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

Jump to bottom
Closed
wants to merge 4 commits into from
Closed

Remove all pipelines #690

wants to merge 4 commits into from

Conversation

TylerLeonhardt
Copy link
Member

@TylerLeonhardt TylerLeonhardt commented Jun 25, 2018 •
edited
Loading

Bringing this into 2.0.0... This removes all the Pipeline references so that we can easily transition to PowerShellStandard.

@TylerLeonhardt TylerLeonhardt requested a review from rjmholt as a code owner June 25, 2018 23:37
rjmholt
rjmholt reviewed Jun 25, 2018
View reviewed changes
src/PowerShellEditorServices/Session/PowerShell3Operations.cs
@@ -32,27 +33,11 @@ public void PauseDebugger(Runspace runspace)
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.

src/PowerShellEditorServices/Session/PowerShellContext.cs
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.

src/PowerShellEditorServices/Session/PowerShellContext.cs Outdated
finally
{
pipeline.Dispose();
return results.FirstOrDefault();
Copy link
Contributor

Choose a reason for hiding this comment

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

Does this make the check above it redundant?

Copy link
Member Author

Choose a reason for hiding this comment

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

no because you can pass in an alternative default value if you'd like.

Copy link
Contributor

Choose a reason for hiding this comment

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

Oh right. Well the two calls to FirstOrDefault still feel a bit redundant, and the first call is only relevant to reference types anyway.

So we could achieve the same with:

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

return results.FirstOrDefault() ?? defaultValue;

Just a thought though

Copy link
Contributor

Choose a reason for hiding this comment

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

Technically the final thing could just be results.First() since having no results there is either impossible or a very strange bug...

Copy link
Collaborator

@SeeminglyScience SeeminglyScience Jun 26, 2018 •
edited
Loading

Choose a reason for hiding this comment

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

How about

results.DefaultIfEmpty(defaultValue).First();

Copy link
Contributor

Choose a reason for hiding this comment

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

Oooh, yes! I thought that was a thing, but never used it before...

Copy link
Member Author

Choose a reason for hiding this comment

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

fixed :)

src/PowerShellEditorServices/Session/PowerShell3Operations.cs
{
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!

src/PowerShellEditorServices/Session/PowerShell3Operations.cs
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... :(

SeeminglyScience
SeeminglyScience approved these changes Jun 26, 2018
View reviewed changes
Copy link
Collaborator

@SeeminglyScience SeeminglyScience left a comment

Choose a reason for hiding this comment

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

LGTM

src/PowerShellEditorServices/Session/PowerShell3Operations.cs
@@ -32,27 +33,11 @@ public void PauseDebugger(Runspace runspace)
out DebuggerResumeAction? debuggerResumeAction)
{
IEnumerable<TResult> executionResult = null;

using (var nestedPipeline = currentRunspace.CreateNestedPipeline())
using (var pwsh = PowerShell.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.

@TylerLeonhardt
Copy link
Member Author

Pretty sure @rjmholt addressed this in #741 so I'm just gonna close it

@dee-see dee-see mentioned this pull request Oct 28, 2018
Closed
@TylerLeonhardt TylerLeonhardt deleted the remove-all-pipelines branch September 16, 2019 16:10
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@rjmholt rjmholt rjmholt approved these changes

@SeeminglyScience SeeminglyScience SeeminglyScience approved these changes

@rkeithhill rkeithhill Awaiting requested review from rkeithhill

Assignees
No one assigned
Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
3 participants
@TylerLeonhardt @rjmholt @SeeminglyScience