Centralise all logging in Chucker#526
Conversation
cortinico
left a comment
There was a problem hiding this comment.
Sorry for the late review but I was 🎅 -ing.
LGTM other than a minor comment 👍
| val cache = cacheDirectoryProvider.provide() | ||
| return if (cache == null) { | ||
| IOException("Failed to obtain a valid cache directory for Chucker transaction file").printStackTrace() | ||
| Logger.warn("Failed to obtain a valid cache directory transaction file") |
There was a problem hiding this comment.
Should we generate an Exception here and call Logger.error instead?
There was a problem hiding this comment.
I removed Exception because I thought it was used only to avoid println as there was no access here to a logger. I'm fine either way and can bring it back if you think it helps.
There was a problem hiding this comment.
I'm unsure about the value added tbh. I'm fine either way 👍
There was a problem hiding this comment.
Ok, I'll leave it then. Just will fix a typo that I noticed.
1c93047 to
ddadc1b
Compare
|
Wanted to take part in review as well 😕 |
|
Sorry, I didn't think this to be an important change to require review from everyone. If you have any comments leave them and I'll make a subsequent PR. :) |
|
Ok, no problem. I thought that changes in classes with our public API would require more maintainers to check. |
📄 Context
Some parts of the code used
printStackTrace()to log failures. I wanted to centralise logging point that can be used in Chucker. This also gives an opportunity to let users set an external logger in the future.📝 Changes
I changed
Loggerto an interface and made an instance of it available inChuckerobject. Companion object ofLoggerdelegates then to this property.I also added a test extension that sets no-op logger. It is needed as we can't have a centralised logger that is not static due to logging in Android components as well.
One more thing – I removed "Chucker" from log messages. It is present in the tag anyway.
📎 Related PR
#452
🚫 Breaking
No.
🛠️ How to test
Nothing special to check. Rely on unit tests.
⏱️ Next steps
N/A