Skip to content

References to RunspaceConfiguration still present #5569

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
Merged
daxian-dbw merged 1 commit into from
Nov 29, 2017
Merged

References to RunspaceConfiguration still present #5569

daxian-dbw merged 1 commit into from
Nov 29, 2017

Conversation

@Bhaal22
Copy link
Contributor

@Bhaal22 Bhaal22 commented Nov 29, 2017

RunspaceConfiguration still mentionned in RunspacePoolInternal ctor API documentation
Removed 2 obsolete test files

Those 2 files should even not compile.

#4942

@Bhaal22
Runspaceconfiguration still mentionned in RunspacePoolInternal ctor A…
221ee83 
…PI documentation

Updated the 2 unit tests accordingly with Runspaceconfiguration removal

 PowerShell#4942
@msftclas
Copy link

msftclas commented Nov 29, 2017 •
edited
Loading

CLA assistant check
All CLA requirements met.

SteveL-MSFT
SteveL-MSFT approved these changes Nov 29, 2017
View reviewed changes
Copy link
Member

@SteveL-MSFT SteveL-MSFT left a comment

Choose a reason for hiding this comment

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

I don't think we currently compile and run those csharp based tests

@Bhaal22
Copy link
Contributor Author

Bhaal22 commented Nov 29, 2017

Indeed. I do not think you compile them ... otherwise they should fail for a couple of weeks.
Initially I removed them ... then I just dediced to remove reference to RunspaceConfiguration and update the AutomationEngine construction with the right parameters.

@SteveL-MSFT
Copy link
Member

SteveL-MSFT commented Nov 29, 2017

@Bhaal22 I think the changes to the test are correct, separate from this PR, someone should understand if that test code is still relevant if so, get it running.

@Bhaal22
Copy link
Contributor Author

Bhaal22 commented Nov 29, 2017

@SteveL-MSFT do you mean you would like to have 2 PRs?

@SteveL-MSFT
Copy link
Member

SteveL-MSFT commented Nov 29, 2017

@Bhaal22 no, I think it's fine to have these changes as part of a single PR. I'm suggesting that investigating the utility of that test code is separate from this PR.

@daxian-dbw daxian-dbw merged commit ad17640 into PowerShell:master Nov 29, 2017
@TravisEz13 TravisEz13 added this to the 6.0.0-GA milestone Nov 30, 2017
TravisEz13 pushed a commit to TravisEz13/PowerShell that referenced this pull request Nov 30, 2017
@Bhaal22 @TravisEz13
Clean up use of 'Runspaceconfiguration' from comments and xunit test …
7b348fa 
…code (PowerShell#5569)
@TravisEz13 TravisEz13 mentioned this pull request Nov 30, 2017
Merged
TravisEz13 pushed a commit that referenced this pull request Nov 30, 2017
@Bhaal22 @TravisEz13
Clean up use of 'Runspaceconfiguration' from comments and xunit test …
a12bd17 
…code (#5569)
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Reviewers

@SteveL-MSFT SteveL-MSFT SteveL-MSFT approved these changes

@BrucePay BrucePay Awaiting requested review from BrucePay

@daxian-dbw daxian-dbw Awaiting requested review from daxian-dbw

@lzybkr lzybkr Awaiting requested review from lzybkr

Assignees

@daxian-dbw daxian-dbw

Labels

None yet

Projects

None yet

Milestone

6.0.0-GA

Development

Successfully merging this pull request may close these issues.

5 participants

@Bhaal22 @msftclas @SteveL-MSFT @daxian-dbw @TravisEz13