Skip to content

LogFactory - GetLogger should null-check logger name#3332

Merged
304NotModified merged 1 commit intoNLog:devfrom
snakefoot:LogFactoryGetLoggerNullCheck
Apr 24, 2019
Merged

LogFactory - GetLogger should null-check logger name#3332
304NotModified merged 1 commit intoNLog:devfrom
snakefoot:LogFactoryGetLoggerNullCheck

Conversation

@snakefoot
Copy link
Copy Markdown
Contributor

@snakefoot snakefoot commented Apr 24, 2019

Throw ArgumentNullException instead of throwing NullReferenceException here (When Name is null):

return ConcreteType.GetHashCode() ^ Name.GetHashCode();

Could also consider doing fallback to string.Empty.

@snakefoot snakefoot changed the title LogFactory - GetLogger should validate name of logger LogFactory - GetLogger should null-check logger name Apr 24, 2019
@codecov-io
Copy link
Copy Markdown

Codecov Report

Merging #3332 into dev will increase coverage by <1%.
The diff coverage is 100%.

@@          Coverage Diff           @@
##             dev   #3332    +/-   ##
======================================
+ Coverage     80%     80%   +<1%     
======================================
  Files        356     356            
  Lines      28101   28103     +2     
  Branches    3743    3744     +1     
======================================
+ Hits       22517   22535    +18     
+ Misses      4503    4486    -17     
- Partials    1081    1082     +1

@304NotModified 304NotModified merged commit ea6f665 into NLog:dev Apr 24, 2019
@304NotModified 304NotModified added this to the 4.6.3 milestone Apr 24, 2019
@304NotModified 304NotModified added enhancement Improvement on existing feature has unittests labels Apr 24, 2019
@304NotModified
Copy link
Copy Markdown
Member

Thanks!

@304NotModified
Copy link
Copy Markdown
Member

Could also consider doing fallback to string.Empty

Doubted also about this, but I think the change in this PR is good enough

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

Labels

enhancement Improvement on existing feature

Projects

None yet

Development

Successfully merging this pull request may close these issues.

3 participants