Suppressing exception thrown when accessing HttpContext.Request #56
Conversation
| } | ||
| catch (HttpException) | ||
| { | ||
| return null; |
There was a problem hiding this comment.
At least a InternalLogger.debug(ex) can be added?
There was a problem hiding this comment.
The compiler refuses to accept InternalLogger.Debug(ex). It will only accept two variants of the Debug method: One taking a string, and the other taking a string and a argument array. Is that as excepted?
There was a problem hiding this comment.
Ah yes that's is added to NLog 4.3.
Debug(Ex.tostring()) is preferred.
There was a problem hiding this comment.
👍 With or without any further explanation?
InternalLogger.Debug("Exception thrown when accessing Request: " + ex);
or
InternalLogger.Debug(ex.ToString());
Do you want a separate test verifying that it is logged?
Current test passes based on the default log level for the internal logger discarding debug statements. A separate test can be added to verify that the internal logger do pick up on the exception if log level is set appropriately.
There was a problem hiding this comment.
The first please :)
No need for testing.
There was a problem hiding this comment.
LOL,
is this a response on "no need for testing"?
There was a problem hiding this comment.
Yes, I was taken by surprise. It's not the response I'm used to ;-)
|
Thanks! |
|
coreclr build is a "bit" broken now ;) |
|
I guess it's time to start looking into this .net core thing then... |
|
Is there a reason why the |
AFAIK we need the |
|
Ok. Glad you could use the code. Hopefully, we can delete our own implementations pretty soon :-) Keep up the good work. |
|
I have question about this PR. Wouldn't we want to implement the same TryGet Method for where we have access the Request? |
As a response to #31
I have not kept the old behavior for .net core.