search

grid-x / modbus

BSD 3-Clause "New" or "Revised" License
78 stars 26 forks source link

chore: Bring back ability to use a custom logger #86

Closed srebhan closed 4 months ago

srebhan commented 4 months ago

When using this project as a modbus library it is beneficial to be able to use a custom logger to e.g. output the information in a common and consistent form or to use sinks not provided by slog.

This PR abstracts the logging to an interface closely following the notion of slog.Logger to allow an easy direct use of slog. As such, the change is fully backward compatible. However, this allows users of the library to define custom loggers.

resolves #79

srebhan commented 4 months ago

@tretmar in case you want to review one more... ;-)

andig commented 4 months ago

It is really unfortunate that https://github.com/grid-x/modbus/pull/80 was merged at all, given its a breaking change. While this PR makes it better, it is still breaking. Please don't get me wrong- I'm fine with breaking changes, but:

I'd suggest to revert #80 and instead a) add a slim interface and b) decide a versioning strategy.

andig commented 4 months ago

The required logger interface for this library would be a single method, not 8. Backwards compatibility is imho not a concern as it was broken with #80 anyway and only very recently.

srebhan commented 4 months ago

@andig while I do agree that #80 is unfortunate I also do understand why you want to differentiate between different log-types especially for the CLI. No matter what is decided at the end, the logger should always be an interface instead of referencing a fixed implementation to allow the user of the library to choose the logging-framework as you want to have a consistent formatting or log to a file as we do in Telegraf.

andig commented 4 months ago

I also do understand why you want to differentiate between different log-types especially for the CLI

Agreed. But the CLI can do more using an slog or whatever logger without complicating matters for library consumers.

No matter what is decided at the end, the logger should always be an interface instead of

Full ack. But let it please not be broader than required. And all that's required is Printf() or even just a func instead of an interface.

srebhan commented 4 months ago

I tend to disagree here, especially with interface vs func... Can you outline how you would suppress debug information with just Printf?

andig commented 4 months ago

Can you outline how you would suppress debug information with just Printf?

Exactly like it was before #80. How you implement the Printf interface is up to the caller. Here‘s what mbmd did and it‘s broken now: https://github.com/volkszaehler/mbmd/blob/master/meters/logger.go

especially with interface vs func

Either would work. Maybe one day Golang will even auto-implement 1-method interfaces by functions.

srebhan commented 4 months ago

@andig I mean how would the modbus library tell you it's a debug message or an error? For a library user this usually is not interesting but for a CLI it might be. Anyway, both ways work for me as long as we can insert our own logging... ;-)

andig commented 4 months ago

Afaikt the library only ever sends debug messages? What else would it send? If you check the current code all logf has simply been replaced by Debug except for the CLI and as consumer CLI can do what it wants but does not depend on the required methods being part of the interface.

If simpler for a quick exchange: meet on Slack?

hnicolaysen commented 4 months ago

Since we just merged #88 which was marked as a replacement for this PR, I'll close it.