Skip to content

Use params in ImportPSModule() definition #7933

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
iSazonov merged 1 commit into from
Oct 5, 2018
Merged

Use params in ImportPSModule() definition #7933

iSazonov merged 1 commit into from
Oct 5, 2018

Conversation

@iSazonov
Copy link
Collaborator

@iSazonov iSazonov commented Oct 3, 2018 •
edited
Loading

PR Summary

Close #6005.

Change method definition
from public void ImportPSModule(string[] name)
to public void ImportPSModule(params string[] name)

PR Checklist

@iSazonov iSazonov self-assigned this Oct 3, 2018
@iSazonov iSazonov requested a review from BrucePay as a code owner October 3, 2018 10:03
@iSazonov
Copy link
Collaborator Author

iSazonov commented Oct 3, 2018

Should we add the change to SDK?
Should we document this in PowerShell-Docs repo?

@iSazonov iSazonov changed the title Use params in ImportPSModule Use params in ImportPSModule() definition Oct 3, 2018
SteveL-MSFT
SteveL-MSFT requested changes Oct 3, 2018
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.

Can we add a test?

@iSazonov
Copy link
Collaborator Author

iSazonov commented Oct 4, 2018

@SteveL-MSFT Could you please clarify - seems we haven't test for public APIs at all, only for cmdlets and other language objects. I don't even know where to put it. I even think that most likely there should have been unit tests.

SteveL-MSFT
SteveL-MSFT approved these changes Oct 4, 2018
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.

Since we don't have existing tests, I won't block this PR. Based on the discussion of concerns of app compat (even though it was agreed it wouldn't be an issue), it would be nice to have some tests to validate.

@iSazonov
Copy link
Collaborator Author

iSazonov commented Oct 5, 2018

@SteveL-MSFT I think we need SDK test set.

@iSazonov iSazonov merged commit 09f5471 into PowerShell:master Oct 5, 2018
@iSazonov
Copy link
Collaborator Author

iSazonov commented Oct 5, 2018

@twitchax Could you please check that the change works as expected and we haven't regression?

@iSazonov iSazonov deleted the use-params-in-importpsmodule branch October 5, 2018 04:32
@twitchax
Copy link

twitchax commented Oct 5, 2018

Awesome. It looks good to me. :)

@iSazonov
Copy link
Collaborator Author

iSazonov commented Oct 5, 2018

@twitchax Can you confirm that you can compile your code after the change?

@twitchax
Copy link

twitchax commented Oct 5, 2018

Yeah: totally fine.

adityapatwardhan pushed a commit to adityapatwardhan/PowerShell that referenced this pull request Apr 9, 2019
@iSazonov
Use params in ImportPSModule (PowerShell#7933)
3b64570
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

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

@adityapatwardhan adityapatwardhan Awaiting requested review from adityapatwardhan

@BrucePay BrucePay Awaiting requested review from BrucePay

Assignees

@iSazonov iSazonov

Labels

None yet

Projects

None yet

Milestone

No milestone

Development

Successfully merging this pull request may close these issues.

ImportPSModule on InitialSessionState should be params.

3 participants

@iSazonov @twitchax @SteveL-MSFT