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
Improve Logger experience #1522
Conversation
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Let's avoid breaking changes coming from LogDestination
and we are good! Great job 👏
04e368f
to
8f3c027
Compare
@@ -66,11 +69,17 @@ open class BaseLogDestination: LogDestination { | |||
self.showFunctionName = showFunctionName | |||
} | |||
|
|||
/// Checks if this destination is enabled for the given level | |||
open func isEnabled(level: LogLevel) -> Bool { | |||
assertionFailure("`isEnabled(level:)` is deprecated, please use `isEnabled(level:subsystem:)`") |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
We preserved the APIs but if customers call super.isEnabled(level)
they'll see a crash when running the app in DEBUG, right?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Yep, they'll immediately spot the crash though
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
LGTM!
- LogConfig changes will now re-create Logger - Introducing `subsystems` to allow finer control in logging
8f3c027
to
5689802
Compare
Codecov Report
@@ Coverage Diff @@
## main #1522 +/- ##
==========================================
- Coverage 88.32% 87.89% -0.43%
==========================================
Files 231 232 +1
Lines 10850 10950 +100
==========================================
+ Hits 9583 9625 +42
- Misses 1267 1325 +58
Flags with carried forward coverage won't be shown. Click here to find out more.
Continue to review full report at Codecov.
|
subsystems
to allow finer control in logging15 minute break PR since I got fed up with uninformative flood of Database log messages. This is used as:
The
subsystem
definition is open for discussion, manually specifying subsystems seems like to lot of burden for developers to not forget. Alternative would be injecting a Logger instance, which required more changes.