infinum / android_dbinspector

Android library for viewing, editing and sharing in app databases.
Apache License 2.0
951 stars 91 forks source link

Unnecessary Timber plant #71

Closed jakoss closed 3 years ago

jakoss commented 3 years ago

:writing_hand: Describe the bug

Here: https://github.com/infinum/android_dbinspector/blob/master/dbinspector/src/main/kotlin/com/infinum/dbinspector/DbInspector.kt#L15-L18 you are using Stump tree on release builds. Is that really necessary? Every time i open DbInspector my logging tree gets bigger with useless tree. Why not just plant debug tree on debug build and else do nothing?

Also - i think you should use some static flag to mark that tree was already planted and not do that again on every inspector open.

And while we're at that - could you make logging optional? I could even do that with simple PR. The reason is quite weird - i have to store logs in local db. I'm using Timber.Tree to do that. And now every time i go in DbInspector i have logs spam about the connection i just made to look at logs table. I would love to just turn that off (or maybe use some TAG for DbInspector, then i could just filter those out

bojankoma commented 3 years ago

I understand your point. I'm not personally happy with Stump idea myself. Can you make a PR about this, and I'll continue upon it with optional logging?

jakoss commented 3 years ago

Here you go: https://github.com/infinum/android_dbinspector/pull/72

bojankoma commented 3 years ago

@jakoss Thanks! Merged it into master and will be available soon in 5.2.1 release.

bojankoma commented 3 years ago

@jakoss How would you do the optional logger part then?

jakoss commented 3 years ago

I think instead of using static Timber.something you could store static nullable instance of Tree. You can then initialize this field with Timber.tag("DBI") on debug build. Then in whole library you can use this field like Logger?.e("error log")

jakoss commented 3 years ago

@bojankoma Can we go back to the optional logger option? Right now i still have db_inspector logs in release app, since i'm planting the tree myself for the app

bojankoma commented 3 years ago

@jakoss I'll reopen this issue and consider it an enhancement. Give me some time please to devise a solution. I'm open to ideas and suggestions too. :)

jakoss commented 3 years ago

My earlier proposal is to use tree planted by yourself, instead of static Timber functions. I think it's reasonable :)

I think instead of using static Timber.something you could store static nullable instance of Tree. You can then initialize this field with Timber.tag("DBI") on debug build. Then in whole library you can use this field like Logger?.e("error log")

bojankoma commented 3 years ago

After a few days of rethinking this and a few more days of research, I've decided to remove Timber completely and introduce a proprietary Logger engine. There shouldn't be any breaking changes for existing integrations. Library initialises with a no-op EmptyLogger and defaults to same in it's only .show() method.
Developer can override that default one (or even the initialiser one with some extra effort) with a custom logger based on abstract Logger, or use AndroidLogger provided by DbInspector library. Logger also provides options to filter out log messages by level (info, debug, error, none).

This should solve constant Timber interferences with, in most cases, instances or trees used in application projects. I'll take a few more days to test it out a little bit, but utmost an updated release will be available on Tuesday, if not earlier.

bojankoma commented 3 years ago

5.3.2 is out with aforementioned changes. Please, let me know if this fits your needs.

jakoss commented 3 years ago

That's perfect, thank you!