Fix incorrect references and improve formatting in PSCommand XML comments#15568
Conversation
|
|
|
@octos4murai Please sign CLA. Without this we can not merge the PR. |
Done. Thanks for the reminder. |
rjmholt
left a comment
There was a problem hiding this comment.
The example changes are much needed, thanks!
The parameter name changes we can't take, and I don't think the terminology changes add clarity here
There was a problem hiding this comment.
Actually more than strict cmdlets can be used here. The path to a script will work as well, for example
There was a problem hiding this comment.
An overload for this method refers to cmdlet in its XML comments summary. Its parameter is also named cmdlet but as you mentioned elsewhere, this would be a breaking change.
Do you suggest to:
- Keep this the same and change the overload method's XML comments to also use
commandto connote that more than strict cmdlets can be used, or - Allow the apparent inconsistency and make no changes?
There was a problem hiding this comment.
Its parameter is also named cmdlet but as you mentioned elsewhere, this would be a breaking change.
Yes, it's an unfortunate inconsistency, but one that we must now live with. The parameter name is part of the API.
Feel free to add any extra documentation around the XML, but the parameter names themselves can't be changed.
There was a problem hiding this comment.
There was a problem hiding this comment.
Noted. I will exclude this breaking change.
There was a problem hiding this comment.
@rjmholt I think there is a bug here where a potential null argument is referred to using string "cmdlet". The actual argument name is not "cmdlet" but rather "command". If you agree, perhaps I can replace this line with:
throw PSTraceSource.NewArgumentNullException(nameof(command));
There was a problem hiding this comment.
Please pull new PR with the change.
There was a problem hiding this comment.
Please pull new PR with the change.
Just would like some clarification -- should I create a new GitHub issue and then create a new PR linked to that issue?
There was a problem hiding this comment.
Issues are need only for discussions to understand what we should do or not do. Here we see simple inconsistency and can accept new PR.
There was a problem hiding this comment.
Issues are need only for discussions to understand what we should do or not do. Here we see simple inconsistency and can accept new PR.
Got it. Thank you.
|
🎉 Handy links: |
PR Summary
Changes parameter name fromcommandtocmdletinAddCommand(string command)to match its overloadAddCommand(string cmdlet, bool useLocalScope)Fixes a bug where the value passed into the constructor for an Argument Null Exception object does not match the name of the relevant parameterPR Context
Closes #15537
PR Checklist
.h,.cpp,.cs,.ps1and.psm1files have the correct copyright headerWIP:or[ WIP ]to the beginning of the title (theWIPbot will keep its status check atPendingwhile the prefix is present) and remove the prefix when the PR is ready.(which runs in a different PS Host).