nictuku / dht

Kademlia/Mainline DHT node in Go.
Other
826 stars 144 forks source link

introduce DebugLogger interface #62

Closed cenkalti closed 6 years ago

cenkalti commented 6 years ago

Ref #61

@nictuku This is the smallest interface that I can think of: https://github.com/cenkalti/dht/blob/29206eba2e98f2d7fab5affe38d5655a78b45db7/logging.go#L3-L7

By implementing this interface people can still use dht with glog or any logger they want.

I have named it as Logger2 because there is already an interface named as Logger: https://github.com/cenkalti/dht/blob/29206eba2e98f2d7fab5affe38d5655a78b45db7/dht.go#L223-L227

Of course this name is unacceptable :)

In fact, existing Logger interface does not represent a logger actually. I suggest renaming it as Hooks or something else and make Logger2 -> Logger. However, this is not possible without breaking backwards compatibility. I think it is best to tag the current commit as v1 and include this change in v2.

nictuku commented 6 years ago

Thanks for the PR. Could you document Logger and Logger2 in the Config struct? Feel free to vocally complain about Logger and say it should be renamed to Hooks 😅.

nictuku commented 6 years ago

For record, this change removes some granularity about log message verbosity. But that's OK since in the worst case scenario (e.g: when debugging) one can just log everything and filter things later.

cenkalti commented 6 years ago

@nictuku I think Logger2 is really a bad name :) Can I rename it as MessageLogger?

nictuku commented 6 years ago

Perhaps DebugLogger ?

cenkalti commented 6 years ago

@nictuku done.

nictuku commented 6 years ago

@cenkalti the change looks good. I have no further comments.

The CI is broken, sadly, so I'll test it out tomorrow or so, then I'll merge.

Thank you again for this Cenk!