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

Apply ArgumentNullException.ThrowIfNull to NLog source-code #889

Merged
merged 7 commits into from
Nov 27, 2022

Conversation

bakgerman
Copy link
Contributor

I try to follow your example exactly as you say.

I had to make if statement for this attribute

#if NETCOREAPP3_1_OR_GREATER
[CallerArgumentExpression("argument")]
#endif

since it is not in NET 461 or CORE 2.x, but other than that I did not find any difficulty.

@codecov-commenter
Copy link

codecov-commenter commented Nov 23, 2022

Codecov Report

Base: 70% // Head: 69% // Decreases project coverage by -1% ⚠️

Coverage data is based on head (d8e566b) compared to base (0176c44).
Patch has no changes to coverable lines.

Additional details and impacted files
@@          Coverage Diff          @@
##           master   #889   +/-   ##
=====================================
- Coverage      70%    69%   -1%     
=====================================
  Files          65     65           
  Lines        1218   1230   +12     
  Branches      308    316    +8     
=====================================
- Hits          850    849    -1     
- Misses        229    240   +11     
- Partials      139    141    +2     
Impacted Files Coverage Δ
.../Shared/LayoutRenderers/AspNetMvcActionRenderer.cs 67% <0%> (-13%) ⬇️
...red/LayoutRenderers/AspNetMvcControllerRenderer.cs 67% <0%> (-13%) ⬇️
...utRenderers/AspNetRequestDurationLayoutRenderer.cs 56% <0%> (-9%) ⬇️
...tRenderers/AspNetRequestRouteParametersRenderer.cs 17% <0%> (+7%) ⬆️

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

☔ View full report at Codecov.
📢 Do you have feedback about the report comment? Let us know in this issue.

@snakefoot
Copy link
Contributor

snakefoot commented Nov 24, 2022

Would it be possible to apply the solution found here: NLog/NLog#5068 ? (Explicit define CallerArgumentExpressionAttribute when not part of target-platform)

Would be nice to get rid of these lines (Maybe always use Guard.ThrowIfNull):

#if NET6_0_OR_GREATER
using ArgumentNullException = System.ArgumentNullException;
#else
using ArgumentNullException = NLog.Web.Internal.ArgumentNullException;
#endif

@bakgerman
Copy link
Contributor Author

Should now be as requested, thank you for the reference into NLog commit.

@pull-request-size pull-request-size bot added size/L and removed size/M labels Nov 25, 2022
@bakgerman
Copy link
Contributor Author

Added unit tests for CallerArgumentExpressionAttribute class

@snakefoot
Copy link
Contributor

Could you make an unit-test that verifies that UseNLog() throws a valid ArgumentNullException with expected ParamName-property ?

@bakgerman
Copy link
Contributor Author

unit test added as requested

@sonarcloud
Copy link

sonarcloud bot commented Nov 26, 2022

Kudos, SonarCloud Quality Gate passed!    Quality Gate passed

Bug A 0 Bugs
Vulnerability A 0 Vulnerabilities
Security Hotspot A 0 Security Hotspots
Code Smell A 0 Code Smells

87.5% 87.5% Coverage
0.0% 0.0% Duplication

@snakefoot snakefoot added this to the 5.2 milestone Nov 27, 2022
@snakefoot
Copy link
Contributor

snakefoot commented Nov 27, 2022

Looks like the internal CallerArgumentExpressionAttribute is working, since .NET Framework v4.6.1 also works.

Thank you for this nice pull-request and the unit-tests.

@snakefoot snakefoot merged commit 41faea4 into NLog:master Nov 27, 2022
@bakgerman bakgerman deleted the argument-null-exception branch November 28, 2022 03:14
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

3 participants