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.

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

Use ArgumentNullException instead null coalescing #51732

Conversation

aligoren
Copy link
Contributor

@aligoren aligoren commented Oct 28, 2023

Use ArgumentNullException instead of null coalescing in AuthenticationState

  • You've read the Contributor Guide and Code of Conduct.
  • You've included unit or integration tests for your change, where applicable.
  • You've included inline docs for your change, where applicable.
  • There's an open issue for the PR that you are making. If you'd like to propose a new feature or change, please open an issue to discuss the change or find an existing issue.

Description

According to the #50665 I have made a small change. With this change, this method will not use null coalescing operators to fire the ArgumentNullException exception.

It will use ArgumentNullException.ThrowIfNull instead of it.

This is similar to #50666

I also asked for these kinds of pull requests.(#51731)

So, I think this will be acceptable even if only one change has been made.

@divyeshio fyi

Test Outputs

These are test results;

  Determining projects to restore...
  All projects are up-to-date for restore.
  Microsoft.AspNetCore.Metadata -> C:\Contribs\aspnetcore\artifacts\bin\Microsoft.AspNetCore.Metadata\Debug\net9
  .0\Microsoft.AspNetCore.Metadata.dll
  Microsoft.AspNetCore.Testing -> C:\Contribs\aspnetcore\artifacts\bin\Microsoft.AspNetCore.Testing\Debug\net9.0
  \Microsoft.AspNetCore.Testing.dll
  Microsoft.AspNetCore.Authorization -> C:\Contribs\aspnetcore\artifacts\bin\Microsoft.AspNetCore.Authorization\
  Debug\net9.0\Microsoft.AspNetCore.Authorization.dll
  Microsoft.AspNetCore.Components -> C:\Contribs\aspnetcore\artifacts\bin\Microsoft.AspNetCore.Components\Debug\
  net9.0\Microsoft.AspNetCore.Components.dll
  Microsoft.AspNetCore.Components.Authorization -> C:\Contribs\aspnetcore\artifacts\bin\Microsoft.AspNetCore.Com
  ponents.Authorization\Debug\net9.0\Microsoft.AspNetCore.Components.Authorization.dll
  Microsoft.AspNetCore.Components.Authorization.Tests -> C:\Contribs\aspnetcore\artifacts\bin\Microsoft.AspNetCo
  re.Components.Authorization.Tests\Debug\net9.0\Microsoft.AspNetCore.Components.Authorization.Tests.dll
Test run for C:\Contribs\aspnetcore\artifacts\bin\Microsoft.AspNetCore.Components.Authorization.Tests\Debug\net9.0\Microsoft.AspNetCore.Components.Authorization.Tests.dll (.NETCoreApp,Version=v9.0)
Microsoft (R) Test Execution Command Line Tool Version 17.9.0-preview-23519-02 (x64)
Copyright (c) Microsoft Corporation.  All rights reserved.

Starting test execution, please wait...
A total of 1 test files matched the specified pattern.

Passed!  - Failed:     0, Passed:    30, Skipped:     0, Total:    30, Duration: 73 ms - Microsoft.AspNetCore.Components.Authorization.Tests.dll (net9.0)
PS C:\Contribs\aspnetcore\src\Components\Authorization\test> 

According to the dotnet#50665 I have made a small change. With this change, this method will not use null coalescing operators to fire the ArgumentNullException exception.

It will use ArgumentNullException.ThrowIfNull instead of it.
@aligoren aligoren requested a review from a team as a code owner October 28, 2023 21:54
@ghost ghost added the area-blazor Includes: Blazor, Razor Components label Oct 28, 2023
@dotnet-issue-labeler dotnet-issue-labeler bot added the area-blazor Includes: Blazor, Razor Components label Oct 28, 2023
@ghost ghost added the community-contribution Indicates that the PR has been added by a community member label Oct 28, 2023
@ghost
Copy link

ghost commented Oct 28, 2023

Thanks for your PR, @aligoren. Someone from the team will get assigned to your PR shortly and we'll get it reviewed.

Copy link
Member

@danmoseley danmoseley left a comment

Choose a reason for hiding this comment

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

Thanks
Looks good so long as @BrennanConroy has no perf concern

@danmoseley danmoseley merged commit 0bffc80 into dotnet:main Oct 30, 2023
26 checks passed
@ghost ghost added this to the 9.0-preview1 milestone Oct 30, 2023
@danmoseley
Copy link
Member

Thanks @aligoren if you are interested in continuing, I suggest doing in chunks of a single assembly and a single type of change. (Eg ThrowIfNull vs other throw helper methods). That should strike a balance between too small to be efficient and too large to review easily.

@ghost
Copy link

ghost commented Oct 30, 2023

Hi @danmoseley. It looks like you just commented on a closed PR. The team will most probably miss it. If you'd like to bring something important up to their attention, consider filing a new issue and add enough details to build context.

@aligoren aligoren deleted the aligoren/use-argumentnullexception-helper-in-authentication-state branch October 30, 2023 08:14
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
area-blazor Includes: Blazor, Razor Components community-contribution Indicates that the PR has been added by a community member
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants