Use nameof operator part2 #13086
Use nameof operator part2 #13086
Conversation
5505fac to
8695fba
Compare
|
@xtqqczze Microsoft.Management.UI.Internal is a frozen code. Please exclude it from the PR. |
|
This code should not be changed. It will not develop, it is not tested. |
8695fba to
361ddb6
Compare
|
@iSazonov Rebased to remove changes to |
src/System.Management.Automation/engine/remoting/fanin/OutOfProcTransportManager.cs
Outdated
Show resolved
Hide resolved
src/Microsoft.PowerShell.Commands.Utility/commands/utility/MatchString.cs
Show resolved
Hide resolved
d8d1a73 to
7c6efc9
Compare
|
|
||
| if (fed.formatEntryInfo == null) | ||
| { | ||
| PSTraceSource.NewArgumentNullException("fed.formatEntryInfo"); |
There was a problem hiding this comment.
I wonder why do we change this? Please revert.
There was a problem hiding this comment.
We should not use ArgumentNullException here as the argument is not null but a property.
There was a problem hiding this comment.
Intention is to trace. We shouldn't remove this.
There was a problem hiding this comment.
I wonder why we do not throw the exception from NewArgumentNullException here. I think this could be a bug.
|
|
||
| if (string.IsNullOrEmpty(Value)) | ||
| if (Value == null || ValueType == DisplayEntryValueType.Property) | ||
| throw PSTraceSource.NewArgumentNullException(nameof(expression)); |
There was a problem hiding this comment.
I wonder why do we change this? Please revert.
There was a problem hiding this comment.
We should not use ArgumentNullException here as the argument expression may not be null.
There was a problem hiding this comment.
Intention is to trace. We shouldn't remove this.
There was a problem hiding this comment.
Documentation for ArgumentNullException states:
An ArgumentNullException exception is thrown when a method is invoked and at least one of the passed arguments is null but should never be null.
The passed argument expression may not be null when we throw here so ArgumentNullException is not appropriate.
| ActionPreference preference = pair.Value; | ||
| if (errorRecord == null) | ||
| { | ||
| throw PSTraceSource.NewArgumentNullException("errorRecord"); |
There was a problem hiding this comment.
Why not NewArgumentNullException(nameof(errorRecord))?
There was a problem hiding this comment.
Method signature is DoWriteError(object obj), errorRecord is not an argument.
There was a problem hiding this comment.
Intention is to trace. We shouldn't remove this.
There was a problem hiding this comment.
We should not pass "errorRecord" as paramName as it is not an argument for this method.
There was a problem hiding this comment.
The question is not for the style PR.
|
This pull request has been automatically marked as stale because it has been marked as requiring author feedback but has not had any activity for 15 days. It will be closed if no further activity occurs within 10 days of this comment. |
|
@xtqqczze Please address last comments. |
|
@iSazonov Addressed |
|
This pull request has been automatically marked as Review Needed because it has been there has not been any activity for 7 days. |
|
@PowerShell/powershell-maintainers reviewed this PR and agreed to accept it. |
|
This pull request has been automatically marked as Review Needed because it has been there has not been any activity for 7 days. |
There was a problem hiding this comment.
Let's start with a PR where all the names end up the change and no types are changed. There is too much too review here.
| /// <param name="str"></param> | ||
| /// <param name="offset"></param> | ||
| /// <returns></returns> | ||
| /// <exception cref="HostException"> | ||
| /// If Win32's WideCharToMultiByte fails | ||
| /// </exception> | ||
|
|
||
| public override | ||
| int LengthInBufferCells(string s, int offset) | ||
| int LengthInBufferCells(string str, int offset) | ||
| { | ||
| if (s == null) | ||
| if (str == null) | ||
| { | ||
| throw PSTraceSource.NewArgumentNullException("str"); | ||
| throw PSTraceSource.NewArgumentNullException(nameof(str)); | ||
| } | ||
|
|
||
| return ConsoleControl.LengthInBufferCells(s, offset, parent.SupportsVirtualTerminal); | ||
| return ConsoleControl.LengthInBufferCells(str, < E7E6 span class="pl-s1">offset, parent.SupportsVirtualTerminal); |
There was a problem hiding this comment.
Please try to keep different changes in different PRs
| if (fed.formatEntryInfo == null) | ||
| { | ||
| PSTraceSource.NewArgumentNullException("fed.formatEntryInfo"); | ||
| PSTraceSource.NewArgumentException(nameof(fed), "fed.formatEntryInfo is null"); |
There was a problem hiding this comment.
Please move any changes which change an exception type to a new PR
| { | ||
| #if CORECLR | ||
| throw PSTraceSource.NewArgumentException("isConfiguration", ParserStrings.ConfigurationNotSupportedInPowerShellCore); | ||
| throw PSTraceSource.NewArgumentException(nameof(moduleInfo), ParserStrings.ConfigurationNotSupportedInPowerShellCore); |
| { | ||
| if (typeDefinition == null) | ||
| throw PSTraceSource.NewArgumentNullException("viewDefinition"); | ||
| throw PSTraceSource.NewArgumentNullException(nameof(typeDefinition)); |
| if (newCmdletInfo == null) | ||
| { | ||
| throw PSTraceSource.NewArgumentNullException("cmdlet"); | ||
| throw PSTraceSource.NewArgumentNullException(nameof(newCmdletInfo)); |
| if (newFunction == null) | ||
| { | ||
| throw PSTraceSource.NewArgumentNullException("function"); | ||
| throw PSTraceSource.NewArgumentNullException(nameof(newFunction)); |
| // a bad format. | ||
|
|
||
| throw PSTraceSource.NewArgumentException("name"); | ||
| throw PSTraceSource.NewArgumentException(nameof(splitName)); |
|
This pull request has been automatically marked as stale because it has been marked as requiring author feedback but has not had any activity for 15 days. It will be closed if no further activity occurs within 10 days of this comment. |
|
@iSazonov I have made changes to this PR, could you please reopen. |
|
I can not reopen :-( - GitHub button is blocked. Do you remove the PR branch? |
|
@xtqqczze Please create new PR. |
|
Opened #13875 |
|
See also #13898 for |
PR Summary
"paramName"->nameof(paramName)Previously this PR had more changes, but they were removed for a future follow-up PR.
PR Context
Follow-up: #12716
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.