Skip to content
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

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

Closed
wants to merge 1 commit into from

Conversation

srebhan
Copy link
Contributor

@srebhan srebhan commented May 2, 2024

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
Copy link
Contributor Author

srebhan commented May 2, 2024

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

@andig
Copy link
Contributor

andig commented May 2, 2024

It is really unfortunate that #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 andig mentioned this pull request May 2, 2024
@andig
Copy link
Contributor

andig commented May 2, 2024

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
Copy link
Contributor Author

srebhan commented May 2, 2024

@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
Copy link
Contributor

andig commented May 2, 2024

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
Copy link
Contributor Author

srebhan commented May 2, 2024

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

@andig
Copy link
Contributor

andig commented May 2, 2024

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.

@andig andig mentioned this pull request May 2, 2024
@srebhan
Copy link
Contributor Author

srebhan commented May 3, 2024

@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
Copy link
Contributor

andig commented May 3, 2024

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
Copy link
Contributor

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

@hnicolaysen hnicolaysen closed this May 3, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Enhancement of Logging System in Modbus Package
3 participants