Skip to content

Change Get-Help cmdlet -Parameter parameter so it accepts string arrays.#8454

Merged
adityapatwardhan merged 11 commits intoPowerShell:masterfrom
sethvs:helpParameter
Jan 16, 2019
Merged

Change Get-Help cmdlet -Parameter parameter so it accepts string arrays.#8454
adityapatwardhan merged 11 commits intoPowerShell:masterfrom
sethvs:helpParameter

Conversation

@sethvs
Copy link
Contributor

@sethvs sethvs commented Dec 12, 2018

PR Summary

Fix #8453

This PR changes Get-Help cmdlet -Parameter parameter so it accepts string arrays.
Also changes help function.

PR Checklist

iSazonov
iSazonov reviewed Dec 12, 2018
View reviewed changes
Copy link
Collaborator

@iSazonov iSazonov left a comment

Choose a reason for hiding this comment

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

We need PowerShell Committee approval to change the public API.

src/System.Management.Automation/help/HelpCommands.cs Outdated
Copy link
Collaborator

@iSazonov iSazonov Dec 12, 2018

Choose a reason for hiding this comment

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

Why we use the private variable? Why not public string[] Parameter { set; get; }

Copy link
Contributor Author

@sethvs sethvs Dec 13, 2018

Choose a reason for hiding this comment

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

Agree.

@iSazonov iSazonov added the Review - Committee The PR/Issue needs a review from the PowerShell Committee label Dec 12, 2018
@adityapatwardhan adityapatwardhan self-assigned this Jan 7, 2019
adityapatwardhan
adityapatwardhan requested changes Jan 8, 2019
View reviewed changes
src/System.Management.Automation/help/HelpCommands.cs Outdated
Copy link
Member

@adityapatwardhan adityapatwardhan Jan 8, 2019

Choose a reason for hiding this comment

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

This block of code seems very similar to code at line # 443. Can you refactor it into a method?

Copy link
Contributor Author

@sethvs sethvs Jan 9, 2019

Choose a reason for hiding this comment

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

Done.

@iSazonov
Copy link
Collaborator

iSazonov commented Jan 9, 2019

@sethvs Please fix style issues (excluding complex method).

@sethvs
Copy link
Contributor Author

sethvs commented Jan 9, 2019

@iSazonov For some reason CodeFactor still shows lines with issues from the first commit. There are no such lines now.

iSazonov
iSazonov reviewed Jan 9, 2019
View reviewed changes
src/System.Management.Automation/help/HelpCommands.cs
if ((cat & supportedCategories) == 0)
{
if (!string.IsNullOrEmpty(Parameter))
if (Parameter != null)
Copy link
Collaborator

@iSazonov iSazonov Jan 9, 2019

Choose a reason for hiding this comment

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

Could you please comment why we need the change?

Copy link
Contributor Author

@sethvs sethvs Jan 9, 2019

Choose a reason for hiding this comment

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

Parameter is a string array now.

@SteveL-MSFT
Copy link
Member

SteveL-MSFT commented Jan 9, 2019

@PowerShell/powershell-committee reviewed this and is ok with the change

@SteveL-MSFT 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 Jan 9, 2019
iSazonov
iSazonov reviewed Jan 10, 2019
View reviewed changes
src/System.Management.Automation/help/HelpCommands.cs
/// <returns>Array of parameter infos.</returns>
private PSObject[] GetParameterInfo(HelpInfo helpInfo)
{
List<PSObject> parameterInfosList = new List<PSObject>(Parameter.Length);
Copy link
Collaborator

@iSazonov iSazonov Jan 10, 2019

Choose a reason for hiding this comment

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

We should consider moving Parameter != null checks to the method.

Copy link
Contributor Author

@sethvs sethvs Jan 10, 2019

Choose a reason for hiding this comment

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

I'm not sure that this is necessary.

@iSazonov @sethvs
Update src/System.Management.Automation/help/HelpCommands.cs
2da2ab3 
Co-Authored-By: sethvs <sethvs@users.noreply.github.com>
@adityapatwardhan
Copy link
Member

adityapatwardhan commented Jan 14, 2019

@sethvs Can you please add tests for the new functionality?

@sethvs
Copy link
Contributor Author

sethvs commented Jan 15, 2019 •
edited
Loading

Error (no failed tests) with macOS check.
Close and reopen to restart.

@sethvs sethvs closed this Jan 15, 2019
@sethvs sethvs reopened this Jan 15, 2019
@sethvs
Copy link
Contributor Author

sethvs commented Jan 15, 2019

macOS check throws Dependency precheck failed!
Again, no failed tests.
Guys, can you take a look into it?

@sethvs sethvs mentioned this pull request Jan 15, 2019
Merged
11 tasks
@adityapatwardhan
Copy link
Member

adityapatwardhan commented Jan 15, 2019

Having a look.

@adityapatwardhan
Copy link
Member

adityapatwardhan commented Jan 15, 2019

It seems that Bootstrap is failing with the following issue: @TravisEz13 any idea?

2019-01-15T17:51:59.5248120Z Warning: openssl 1.0.2q is already installed and up-to-date
2019-01-15T17:51:59.5248480Z To reinstall 1.0.2q, run `brew reinstall openssl`
2019-01-15T17:51:59.5424690Z Error: cmake 3.13.2 is already installed
2019-01-15T17:51:59.5424850Z To upgrade to 3.13.3, run `brew upgrade cmake`
2019-01-15T17:52:01.9032320Z Warning: curl 7.63.0 is already installed and up-to-date
2019-01-15T17:52:01.9032780Z To reinstall 7.63.0, run `brew reinstall curl`
2019-01-15T17:52:06.2156000Z Successfully installed cabin-0.9.0
2019-01-15T17:52:06.2156670Z Successfully installed backports-3.11.4
2019-01-15T17:52:06.2157410Z Successfully installed arr-pm-0.0.10
2019-01-15T17:52:06.2157950Z Successfully installed clamp-1.0.1
2019-01-15T17:52:06.2158020Z Building native extensions. This could take a while...
2019-01-15T17:53:06.0164980Z Before reporting this, could you check that the file you're documenting
2019-01-15T17:53:06.0165390Z has proper syntax:
2019-01-15T17:53:06.0165830Z 
2019-01-15T17:53:06.0166810Z   /usr/local/opt/ruby/bin/ruby -c lib/childprocess/tools/generator.rb
2019-01-15T17:53:06.0167080Z 
2019-01-15T17:53:06.0167210Z RDoc is not a full Ruby parser and will fail when fed invalid ruby programs.
2019-01-15T17:53:06.0167280Z 
2019-01-15T17:53:06.0167360Z The internal error was:
2019-01-15T17:53:06.0167540Z 
2019-01-15T17:53:06.0168380Z 	(NoMethodError) undefined method `[]' for nil:NilClass
2019-01-15T17:53:06.0168460Z 
2019-01-15T17:53:06.0168880Z ERROR:  While executing gem ... (NoMethodError)
2019-01-15T17:53:06.0169910Z     undefined method `[]' for nil:NilClass
2019-01-15T17:53:06.0245480Z Successfully installed ffi-1.10.0

@adityapatwardhan adityapatwardhan merged commit 279993b into PowerShell:master Jan 16, 2019
@adityapatwardhan adityapatwardhan added the CL-General Indicates that a PR should be marked as a general cmdlet change in the Change Log label Jan 16, 2019
@sethvs sethvs deleted the helpParameter branch January 16, 2019 08:09
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Reviewers

@iSazonov iSazonov iSazonov left review comments

@adityapatwardhan adityapatwardhan adityapatwardhan approved these changes

@BrucePay BrucePay Awaiting requested review from BrucePay

@daxian-dbw daxian-dbw Awaiting requested review from daxian-dbw daxian-dbw is a code owner

@Francisco-Gamino Francisco-Gamino Awaiting requested review from Francisco-Gamino

Assignees

@adityapatwardhan adityapatwardhan

Labels

CL-General Indicates that a PR should be marked as a general cmdlet change in the Change Log Committee-Reviewed PS-Committee has reviewed this and made a decision

Projects

None yet

Milestone

No milestone

Development

Successfully merging this pull request may close these issues.

Allow Get-Help cmdlet -Parameter parameter to accept string array.

4 participants

@sethvs @iSazonov @SteveL-MSFT @adityapatwardhan

Comments