Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Jump to bottom

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

Merged
merged 1 commit into from
Jul 6, 2021
Merged

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

merged 1 commit into from
Jul 6, 2021

Conversation

gukoff
Copy link
Contributor

@gukoff gukoff commented Jun 18, 2021 •
edited

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
}

if (parent == null)
{
throw new ArgumentNullException("element");
throw new ArgumentNullException(nameof(parent));
Copy link
Contributor Author

@gukoff gukoff Jun 18, 2021 •
edited

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
Collaborator

@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
Contributor

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
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
Contributor

CodeFactor "Complex Method" new issues are false positives.

iSazonov
iSazonov reviewed Jun 21, 2021
View reviewed changes
src/Microsoft.PowerShell.ScheduledJob/ScheduledJobTrigger.cs Outdated
@@ -711,7 +711,7 @@ public override bool CanConvertFrom(object sourceValue, Type destinationType)
{
if (destinationType == null)
{
throw new ArgumentNullException("destinationType");
throw new ArgumentNullException(nameof(destinationType));
Copy link
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.

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

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
Member

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
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
Contributor

xtqqczze commented Jul 4, 2021 •
edited

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

daxian-dbw
daxian-dbw approved these changes Jul 5, 2021
View reviewed changes
Copy link
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
Contributor

xtqqczze commented Jul 5, 2021

LGTM

@daxian-dbw daxian-dbw merged commit 8dcd5a2 into PowerShell:master Jul 6, 2021
@gukoff
Copy link
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

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

@rkeithhill rkeithhill rkeithhill approved these changes

@iSazonov iSazonov iSazonov approved these changes

@xtqqczze xtqqczze xtqqczze approved these changes

@adityapatwardhan adityapatwardhan Awaiting requested review from adityapatwardhan adityapatwardhan is a code owner

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.

None yet
6 participants
@gukoff @iSazonov @xtqqczze @daxian-dbw @rkeithhill @rjmholt