Skip to content

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

Merged
daxian-dbw merged 1 commit intoPowerShell:masterfrom
gukoff:ArgumentNullException-with-nameof
Jul 6, 2021
Merged

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

Conversation

@gukoff
Copy link
Copy Markdown
Contributor

@gukoff gukoff commented Jun 18, 2021 •
edited
Loading

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

@ghost ghost assigned daxian-dbw Jun 18, 2021
gukoff
gukoff commented Jun 18, 2021
View reviewed changes
src/Microsoft.Management.UI.Internal/ManagementList/Common/WpfHelp.cs Outdated
Copy link
Copy Markdown
Contributor Author

@gukoff gukoff Jun 18, 2021 •
edited
Loading

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
Copy link
Copy Markdown
Collaborator

iSazonov commented Jun 18, 2021

@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
Copy link
Copy Markdown
Contributor

xtqqczze commented Jun 20, 2021

See also stale PR: #13875

@gukoff gukoff force-pushed the ArgumentNullException-with-nameof branch from 8ca1bdd to 484508a Compare June 21, 2021 07:30
@gukoff
Copy link
Copy Markdown
Contributor Author

gukoff commented Jun 21, 2021

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 gukoff marked this pull request as ready for review June 21, 2021 07:43
@xtqqczze
Copy link
Copy Markdown
Contributor

xtqqczze commented Jun 21, 2021

CodeFactor "Complex Method" new issues are false positives.

iSazonov
iSazonov reviewed Jun 21, 2021
View reviewed changes
src/Microsoft.PowerShell.ScheduledJob/ScheduledJobTrigger.cs Outdated
Copy link
Copy Markdown
Collaborator

@iSazonov iSazonov Jun 21, 2021

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
daxian-dbw
daxian-dbw approved these changes Jun 22, 2021
View reviewed changes
Copy link
Copy Markdown
Member

@daxian-dbw daxian-dbw left a comment

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
Copy link
Copy Markdown

ghost commented Jul 2, 2021

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 daxian-dbw removed the Review - Needed The PR is being reviewed label Jul 4, 2021
@daxian-dbw
Copy link
Copy Markdown
Member

daxian-dbw commented Jul 4, 2021

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

@iSazonov
Copy link
Copy Markdown
Collaborator

iSazonov commented Jul 4, 2021

ConciseView test fails.

@gukoff gukoff force-pushed the ArgumentNullException-with-nameof branch from ab3435b to 7e7ef2b Compare July 4, 2021 16:20
@gukoff gukoff force-pushed the ArgumentNullException-with-nameof branch from 7e7ef2b to a2a4f5b Compare July 4, 2021 16:41
@xtqqczze
Copy link
Copy Markdown
Contributor

xtqqczze commented Jul 4, 2021 •
edited
Loading

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

daxian-dbw
daxian-dbw approved these changes Jul 5, 2021
View reviewed changes
Copy link
Copy Markdown
Member

@daxian-dbw daxian-dbw left a comment

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
Copy link
Copy Markdown
Contributor

xtqqczze commented Jul 5, 2021

LGTM

@daxian-dbw daxian-dbw merged commit 8dcd5a2 into PowerShell:master Jul 6, 2021
@gukoff
Copy link
Copy Markdown
Contributor Author

gukoff commented Jul 6, 2021

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 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
Copy link
Copy Markdown

ghost commented Jul 22, 2021

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

Handy links:

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Reviewers

@daxian-dbw daxian-dbw daxian-dbw approved these changes

@iSazonov iSazonov iSazonov approved these changes

@adityapatwardhan adityapatwardhan Awaiting requested review from adityapatwardhan

+2 more reviewers

@rkeithhill rkeithhill rkeithhill approved these changes

@xtqqczze xtqqczze xtqqczze approved these changes

Reviewers whose approvals may not affect merge requirements

Assignees

@daxian-dbw 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