Skip to content

Throw ArgumentNullException with nameof(param), not "param" - #15604

Merged
Dongbo Wang (daxian-dbw) merged 1 commit into
PowerShell:masterfrom
gukoff:ArgumentNullException-with-nameof
Jul 6, 2021
Merged

Throw ArgumentNullException with nameof(param), not "param"#15604
Dongbo Wang (daxian-dbw) merged 1 commit into
PowerShell:masterfrom
gukoff:ArgumentNullException-with-nameof

Conversation

@gukoff

@gukoff Konstantin Gukov (gukoff) commented Jun 18, 2021 •
edited
Loading

Copy link
Copy Markdown
Contributor

PR Summary

Refactoring of ArgumentNullException-s.

It covers a few files but not the whole solution.

Similar effort in the past: #13875

PR Context

Follow the best practice of throwing argument exceptions: https://www.jetbrains.com/help/resharper/UseNameofExpression.html

PR Checklist

gukoff
Konstantin Gukov (gukoff) commented Jun 18, 2021
View reviewed changes
Comment thread src/Microsoft.Management.UI.Internal/ManagementList/Common/WpfHelp.cs Outdated

@gukoff Konstantin Gukov (gukoff) Jun 18, 2021 •
edited
Loading

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

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

I believe this was a copy-paste mistake.

@iSazonov

Ilya (iSazonov) commented Jun 18, 2021

Copy link
Copy Markdown
Collaborator

Konstantin Gukov (@gukoff) Thanks for your contribution!

Code under Microsoft.Management.UI.Internal is frozen and shouldn't be changed. The same for remoting code (WinRM).

@xtqqczze

xtqqczze commented Jun 20, 2021

Copy link
Copy Markdown
Contributor

See also stale PR: #13875

@gukoff
Konstantin Gukov (gukoff) force-pushed the ArgumentNullException-with-nameof branch from 8ca1bdd to 484508a Compare June 21, 2021 07:30
@gukoff

Konstantin Gukov (gukoff) commented Jun 21, 2021

Copy link
Copy Markdown
Contributor Author

Code under Microsoft.Management.UI.Internal is frozen and shouldn't be changed. The same for remoting code (WinRM).

Removed UI.Internal and remoting/common from the PR.

See also stale PR: #13875

If It looks like the overlap with #13875 is minimal - only in SessionBasedWrapper.cs and GetCommandCommand.cs. Also this PR adds null coalescing in those files, a nice touch beyond just nameof-s.

To me, it makes sense to merge this one and then return to #13875 where a lot of similar work has been done.

@gukoff
Konstantin Gukov (gukoff) marked this pull request as ready for review June 21, 2021 07:43
@xtqqczze

xtqqczze commented Jun 21, 2021

Copy link
Copy Markdown
Contributor

CodeFactor "Complex Method" new issues are false positives.

xtqqczze
xtqqczze reviewed Jun 21, 2021
View reviewed changes
Comment thread src/Microsoft.PowerShell.CoreCLR.Eventing/DotNetCode/Eventing/EventProvider.cs Outdated
iSazonov
Ilya (iSazonov) reviewed Jun 21, 2021
View reviewed changes
Comment thread src/Microsoft.PowerShell.ScheduledJob/ScheduledJobTrigger.cs Outdated

@iSazonov Ilya (iSazonov) Jun 21, 2021

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

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

Microsoft.PowerShell.ScheduledJob is not compiled in the repo and we can not accept the changes.

xtqqczze
xtqqczze reviewed Jun 21, 2021
View reviewed changes
Comment thread src/System.Management.Automation/engine/GetCommandCommand.cs Outdated
daxian-dbw
Dongbo Wang (daxian-dbw) approved these changes Jun 22, 2021
View reviewed changes

@daxian-dbw Dongbo Wang (daxian-dbw) left a comment

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

LGTM

@ghost ghost added the Review - Needed The PR is being reviewed label Jul 2, 2021
@ghost

Deleted user (ghost) commented Jul 2, 2021

Copy link
Copy Markdown

This pull request has been automatically marked as Review Needed because it has been there has not been any activity for 7 days.
Maintainer, please provide feedback and/or mark it as Waiting on Author

@daxian-dbw Dongbo Wang (daxian-dbw) removed the Review - Needed The PR is being reviewed label Jul 4, 2021
@daxian-dbw

Dongbo Wang (daxian-dbw) commented Jul 4, 2021

Copy link
Copy Markdown
Member

It's not clear why the error view tests would fail. Re-run the corresponding jobs to see if it's intermittent.

@iSazonov

Ilya (iSazonov) commented Jul 4, 2021

Copy link
Copy Markdown
Collaborator

ConciseView test fails.

@gukoff
Konstantin Gukov (gukoff) force-pushed the ArgumentNullException-with-nameof branch from ab3435b to 7e7ef2b Compare July 4, 2021 16:20
@gukoff
Konstantin Gukov (gukoff) force-pushed the ArgumentNullException-with-nameof branch from 7e7ef2b to a2a4f5b Compare July 4, 2021 16:41
@xtqqczze

xtqqczze commented Jul 4, 2021 •
edited
Loading

Copy link
Copy Markdown
Contributor

Konstantin Gukov (@gukoff) Please could you rebase back onto ab3435b436835d488ee6ba32a4a6c994d7ed8efd to ease review, if possible.

daxian-dbw
Dongbo Wang (daxian-dbw) approved these changes Jul 5, 2021
View reviewed changes

@daxian-dbw Dongbo Wang (daxian-dbw) left a comment

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

I'm fine keeping the change as is. The fact that a few ConciseView tests failed after changing to use new ArgumentNullException() in properties indicates that change could be breaking (though it shouldn't ...).

xtqqczze If you are interested, please go ahead making that change for the properties, and see if ConciseView has an unexpected dependency somehow.

@xtqqczze xtqqczze mentioned this pull request Jul 5, 2021
Closed
@xtqqczze xtqqczze mentioned this pull request Jul 5, 2021
Closed
@xtqqczze

xtqqczze commented Jul 5, 2021

Copy link
Copy Markdown
Contributor

LGTM

@daxian-dbw
Dongbo Wang (daxian-dbw) merged commit 8dcd5a2 into PowerShell:master Jul 6, 2021
@gukoff

Konstantin Gukov (gukoff) commented Jul 6, 2021

Copy link
Copy Markdown
Contributor Author

As a side note - don't you want add a PR check that there's no diff in Microsoft.Management.UI.Internal / remoting/common / Microsoft.PowerShell.ScheduledJob / etc ?

@rjmholt Rob Holt (rjmholt) added the CL-Engine Indicates that a PR should be marked as an engine change in the Change Log label Jul 21, 2021
@ghost

Deleted user (ghost) commented Jul 22, 2021

Copy link
Copy Markdown

🎉v7.2.0-preview.8 has been released which incorporates this pull request.:tada:

Handy links:

Thatgfsj (Thatgfsj) pushed a commit to Thatgfsj/PowerShell that referenced this pull request Aug 6, 2026
@gukoff
Use nameof to get parameter names when creating `ArgumentNullExcept…
2a3a4a6 
…ion` (PowerShell#15604)
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Reviewers

@daxian-dbw Dongbo Wang (daxian-dbw) daxian-dbw approved these changes
@iSazonov Ilya (iSazonov) iSazonov approved these changes
@adityapatwardhan Aditya Patwardhan (adityapatwardhan) Awaiting requested review from adityapatwardhan
+2 more reviewers
@rkeithhill Keith Hill (rkeithhill) rkeithhill approved these changes
@xtqqczze xtqqczze xtqqczze approved these changes
Reviewers whose approvals may not affect merge requirements

Assignees

@daxian-dbw Dongbo Wang (daxian-dbw)

Labels

CL-Engine Indicates that a PR should be marked as an engine change in the Change Log

Projects

None yet

Milestone

No milestone

Development

Successfully merging this pull request may close these issues.

6 participants

@gukoff @iSazonov @xtqqczze @daxian-dbw @rkeithhill @rjmholt