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

Introduce simplified null check [debatable] #2780

Open
wants to merge 11 commits into
base: master
Choose a base branch
from
Open

Introduce simplified null check [debatable] #2780

wants to merge 11 commits into from

Conversation

sungam3r
Copy link
Member

@sungam3r sungam3r commented Dec 23, 2021 •
edited

If OK then I'll copy-paste this approach in more places.

In the meantime waiting for dotnet/csharplang#2145

UPDATE
Available solution for NET6 is ArgumentNullException.ThrowIfNull.

@sungam3r sungam3r added the enhancement New feature or request label Dec 23, 2021
@sungam3r sungam3r added this to the 4.7 milestone Dec 23, 2021
@sungam3r sungam3r self-assigned this Dec 23, 2021
@Shane32
Copy link
Member

Shane32 commented Dec 23, 2021

I already described why I cannot endorse these changes.

@sungam3r sungam3r added the code style Pull request that changes or applies code style rules label Dec 23, 2021
@sungam3r sungam3r changed the title Introduce simplified null check Introduce simplified null check [debatable] Dec 23, 2021
@sungam3r sungam3r removed this from the 4.7 milestone Dec 23, 2021
@Shane32 Shane32 mentioned this pull request Dec 23, 2021
Merged
@sungam3r
Copy link
Member Author

Release of !! feature was postponed. Not in C# 11.

@sungam3r
Copy link
Member Author

sungam3r commented Nov 28, 2022 •
edited

Examples of implementation in other repos

@sungam3r
Merge branch 'master' of github.com:graphql-dotnet/graphql-dotnet int…
7975a9e 
…o expression

# Conflicts:
#	src/Directory.Build.props
#	src/GraphQL.MemoryCache/MemoryDocumentCache.cs
@codecov-commenter
Copy link

Codecov Report

Merging #2780 (b1dca66) into master (831efff) will decrease coverage by 0.03%.
The diff coverage is 33.33%.

@@            Coverage Diff             @@
##           master    #2780      +/-   ##
==========================================
- Coverage   83.64%   83.61%   -0.04%     
==========================================
  Files         377      378       +1     
  Lines       16378    16391      +13     
  Branches     2635     2635              
==========================================
+ Hits        13700    13705       +5     
- Misses       2056     2065       +9     
+ Partials      622      621       -1
Impacted Files Coverage Δ
src/CallerArgumentExpressionAttribute.cs 23.07% <23.07%> (ø)
src/GraphQL.MemoryCache/MemoryDocumentCache.cs 56.52% <100.00%> (+4.34%) ⬆️

Help us with your feedback. Take ten seconds to tell us how you rate us. Have a feature suggestion? Share it here.

@Shane32
Copy link
Member

Shane32 commented Jan 7, 2024

I suggest closing this PR.

@gao-artur
Copy link
Contributor

@Shane32, what are your objections? I really like the concise form of the ArgumentNullException.ThrowIfNull. Unfortunately, it's not available in older TFM's, but Guard.ThrowIfNull can be a good alternative.

@Shane32
Copy link
Member

Shane32 commented Jan 7, 2024 •
edited

Well, I guess I'm just not a fan of it. ?? throw new ArgumentNullException(nameof(arg)) seems really simple. Breakpoints are going to occur within the ThrowIfNull method, messing up the call stack... although if we move the throw to a separate method then the null check should get inlined. Another benefit could be that small methods might get inlined where as before they might not have been. So there are some benefits.

@gao-artur
Copy link
Contributor

I'm not sure if these are cons or pros. 😄
Btw, starting NET7 there is an ArgumentException.ThrowIfNullOrEmpty that is similar to throwOnEmptyString in this PR.

@gao-artur
Copy link
Contributor

And I'm happy the !! was not added. C# starts looking like Predator's language.

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

@Shane32 Shane32 Awaiting requested review from Shane32

Assignees

@sungam3r sungam3r

Labels
code style Pull request that changes or applies code style rules enhancement New feature or request
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.

None yet
4 participants
@sungam3r @Shane32 @codecov-commenter @gao-artur