Add S.M.A.PowerShell.Create method overload with Runspace argument by KirkMunro · Pull Request #8057 · PowerShell/PowerShell

KirkMunro · 2018-10-17T02:36:38Z

PR Summary

When invoking multiple PowerShell commands one at a time from an assembly that does not have PowerShell loaded by default, developers should create a runspace first via RunspaceFactory.Create inside of a using statement, and then for each command they want to invoke, create a PowerShell instance via PowerShelll.Create from within another using statement, and then after that set the runspace for the PowerShell instance to the runspace they created from the RunspaceFactory. This is cumbersome and unintuitive, and really should be simplified to make it easier to use and understand.

This PR adds a new PowerShell.Create method overload that accepts a Runspace object so that it is easier for developers to follow this model in their code.

PR Checklist

iSazonov · 2018-10-17T07:02:21Z

@KirkMunro It would be great if you would link two gists with samples before and after the change to demo benefits.

KirkMunro · 2018-10-17T11:22:20Z

@iSazonov Good idea: https://gist.github.com/KirkMunro/9288214d59a89b965974852eb985eff5.

adityapatwardhan · 2018-10-18T19:00:02Z

@KirkMunro I believe this can be done using the overload for IntialSessionState. Which in effect creates a runspace and opens it. Am I missing something?

PowerShell/src/System.Management.Automation/engine/hostifaces/PowerShell.cs

Lines 853 to 861 in 4e5e390

    
           public static PowerShell Create(InitialSessionState initialSessionState) 
        
           { 
        
               PowerShell result = Create(); 
        
               result.Runspace = RunspaceFactory.CreateRunspace(initialSessionState); 
        
               result.Runspace.Open(); 
        
               return result; 
        
           }

KirkMunro · 2018-10-18T19:33:49Z

@adityapatwardhan That only works for one invocation. A single PowerShell instance. The PR allows you to create your runspace and initialize it with the initialSessionState, and then easily use it over, and over, for as many PowerShell instances as you need, keeping each instance in the same runspace that is initialized only once.

src/System.Management.Automation/engine/hostifaces/PowerShell.cs

SteveL-MSFT · 2018-10-24T22:39:51Z

@PowerShell/powershell-committee is ok with this change to have an overloaded api rather than requiring setting runspace on the property

KirkMunro · 2018-10-27T00:22:54Z

FYI, I'm on vacation this week and through the weekend, so I'll catch up on all of this when I am back at home.

PaulHigin

LGTM

test/powershell/engine/Api/BasicEngine.Tests.ps1

Co-Authored-By: KirkMunro <KirkMunro@users.noreply.github.com>

…thub.com/KirkMunro/PowerShell into powershell-create-with-runspace-overload

stale · 2018-12-15T21:17:17Z

This PR has been automatically marked as stale because it has not had activity in the last 30 days. It will be closed if no further activity occurs within 10 days.
Thank you for your contributions.
Community members are welcome to grab these works.

KirkMunro · 2018-12-17T15:44:32Z

@SteveL-MSFT What's the hold up on this one? Is it simply outstanding code reviews?

SteveL-MSFT · 2018-12-20T04:45:23Z

@KirkMunro Yes, please address the code review feedback. Otherwise, it's good to go.

KirkMunro · 2018-12-20T17:49:22Z

@SteveL-MSFT Unless I'm missing something there are no outstanding code review changes to make. See the discussion between Paul, Ilya and I.

SteveL-MSFT · 2018-12-20T18:51:58Z

@KirkMunro you are correct. Unfortunately we're a bit understaffed right now due to the holidays. I expect this to be merged after the new year.

@adityapatwardhan should review your changes and approve

src/System.Management.Automation/engine/hostifaces/PowerShell.cs

adityapatwardhan · 2019-01-07T17:59:01Z

@daxian-dbw Could you update your review please?

daxian-dbw

LGTM

adityapatwardhan · 2019-01-07T19:05:30Z

@daxian-dbw I merged the change. I thought it was assigned to me.

add Create overload with Runspace argument

20058cb

KirkMunro requested review from BrucePay and daxian-dbw as code owners October 17, 2018 02:36

CodeFactor changes

034dda7

iSazonov added the Review - Committee The PR/Issue needs a review from the PowerShell Committee label Oct 17, 2018

adityapatwardhan requested changes Oct 18, 2018

View reviewed changes

src/System.Management.Automation/engine/hostifaces/PowerShell.cs Show resolved Hide resolved

PaulHigin suggested changes Oct 23, 2018

View reviewed changes

src/System.Management.Automation/engine/hostifaces/PowerShell.cs Show resolved Hide resolved

SteveL-MSFT added Committee-Reviewed PS-Committee has reviewed this and made a decision and removed Review - Committee The PR/Issue needs a review from the PowerShell Committee labels Oct 24, 2018

KirkMunro added 4 commits November 4, 2018 15:33

Merge branch 'master' into powershell-create-with-runspace-overload

089662b

added null check and Pester test

51b124b

codefactor changes

15f5d16

codefactor changes

8a84e36

PaulHigin approved these changes Nov 5, 2018

View reviewed changes

adityapatwardhan reviewed Nov 12, 2018

View reviewed changes

test/powershell/engine/Api/BasicEngine.Tests.ps1 Outdated Show resolved Hide resolved

adityapatwardhan and others added 4 commits November 12, 2018 15:52

Code review change

2c176bb

Co-Authored-By: KirkMunro <KirkMunro@users.noreply.github.com>

Merge branch 'master' into powershell-create-with-runspace-overload

f02b4c7

Merge branch 'powershell-create-with-runspace-overload' of https://gi…

ae8f903

…thub.com/KirkMunro/PowerShell into powershell-create-with-runspace-overload

fixed pester test

10c0404

stale bot added Stale and removed Stale labels Dec 15, 2018

iSazonov assigned daxian-dbw Dec 20, 2018

daxian-dbw requested changes Dec 20, 2018

View reviewed changes

src/System.Management.Automation/engine/hostifaces/PowerShell.cs Outdated Show resolved Hide resolved

src/System.Management.Automation/engine/hostifaces/PowerShell.cs Show resolved Hide resolved

src/System.Management.Automation/engine/hostifaces/PowerShell.cs Outdated Show resolved Hide resolved

KirkMunro added 2 commits December 21, 2018 10:29

Merge branch 'master' into powershell-create-with-runspace-overload

432733a

added comments per code review

e350008

adityapatwardhan approved these changes Jan 7, 2019

View reviewed changes

daxian-dbw approved these changes Jan 7, 2019

View reviewed changes

adityapatwardhan merged commit 23f6e8f into PowerShell:master Jan 7, 2019

adityapatwardhan self-assigned this Jan 7, 2019

KirkMunro deleted the powershell-create-with-runspace-overload branch January 7, 2019 21:21

adityapatwardhan added the CL-Engine Indicates that a PR should be marked as an engine change in the Change Log label Jan 7, 2019

Conversation

KirkMunro commented Oct 17, 2018

PR Summary

PR Checklist

Uh oh!

iSazonov commented Oct 17, 2018 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

KirkMunro commented Oct 17, 2018

Uh oh!

adityapatwardhan commented Oct 18, 2018

Uh oh!

KirkMunro commented Oct 18, 2018

Uh oh!

Uh oh!

Uh oh!

SteveL-MSFT commented Oct 24, 2018 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

KirkMunro commented Oct 27, 2018

Uh oh!

PaulHigin left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

stale bot commented Dec 15, 2018

Uh oh!

KirkMunro commented Dec 17, 2018

Uh oh!

SteveL-MSFT commented Dec 20, 2018

Uh oh!

KirkMunro commented Dec 20, 2018

Uh oh!

SteveL-MSFT commented Dec 20, 2018

Uh oh!

Uh oh!

Uh oh!

Uh oh!

adityapatwardhan commented Jan 7, 2019

Uh oh!

daxian-dbw left a comment

Choose a reason for hiding this comment

Uh oh!

adityapatwardhan commented Jan 7, 2019

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

6 participants

iSazonov commented Oct 17, 2018 •

edited

Loading

SteveL-MSFT commented Oct 24, 2018 •

edited

Loading