Feature/remove unnecessary string handling in logging #1103
Conversation
The DebugLogging class does not do anything useful unless compiled using DEBUG mode. Nuget packages are compiled in release mode which only leaves the string formating code without doing anything with the result and if a developer try to use this class from their application code it will be confusing and misleading since it actually does not log anything.
Nuget packages are compiled in release mode so the DebugLogger will not work. Use EmptyLogger as default if not compiling in debug mode.
Nuget packages are compiled in release mode so the DebugLogger will not work. Use EmptyLogger as default if not compiling in debug mode.
|
@fredrikhaglund, |
Source/Prism/Logging/DebugLogger.cs
Outdated
| @@ -5,6 +5,7 @@ | |||
|
|
|||
| namespace Prism.Logging | |||
| { | |||
| #if DEBUG | |||
There was a problem hiding this comment.
Remove this check. This will break anyone using this class as Prism is not released in Debug mode, so this class will now be missing from the assembly.
|
You are aware that this class does not work at all unless compiled in debug mode? Debug.WriteLine is removed by the compiler when compiled in release mode. My whole point is that I used this class in my project and wasted time figuring out why nothing was logged. Another option that does not introduce a breaking change by removing the class would be to add an obsolete attribute with a warning when compiled in release mode. Better? |
|
I've thought about this a little bit and probably the best approach would be to use |
|
Let me think about this a little more. Maybe we should remove the DebugLogger. I did it to make my life easier for developing, but it provides no value to anyone using the class outside of debugging the source code. |
|
I think perhaps the better idea would be to add the DebugLogger to the project templates. This would solve a few problems for developers... |
|
That's actually not a bad idea. |
The DebugLogging class does not do anything useful unless compiled using DEBUG mode. Nuget packages are compiled in release mode which only leaves the string formating code without doing anything with the result and if a developer try to use this class from their application code it will be confusing and misleading since it actually does not log anything.
|
I added #define DEBUG and removed conditional compilation to DebugLogger class. Removing the class or hiding it with a conditional compile would be a breaking change so this is better. |
|
With the DEBUG defined, there should be no need to have the conditional EmptyLogger now right? |
|
Changing so the default is always the EmptyLogger and updating unit test to use DebugLogger? Leaving the DebugLogger as default will cause unneeded string handling and debug logging in release builds of Apps. Not a big deal but everything that can help improve performance in mobile apps is welcome... |
|
As I recall |
|
Thanks for the PR |
|
This thread has been automatically locked since there has not been any recent activity after it was closed. Please open a new issue for related bugs. |
Please take a moment to fill out the following:
Fixes issue #1089.
Changes proposed in this pull request: