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

Add logging configuration #37

Open
moritz157 opened this issue Oct 17, 2024 · 2 comments
Open

Add logging configuration #37

moritz157 opened this issue Oct 17, 2024 · 2 comments
Labels
enhancement New feature or request

Comments

@moritz157
Copy link

moritz157 commented Oct 17, 2024

This might be a kind of minor issue, but I think it would be both useful and good practice to add an option to disable the logged messages. In my opinion there really shouldn't be a bunch of non-configurable print statements in a package like this. However it's obviously very useful to have these messages for debugging purposes. Therefore I propose to, by default, only print them when the app is in debug mode but to provide an option to also force them to be disabled/enabled.

@pratikbaid3
Copy link
Owner

That is a good suggestion. Instead of having it only print in debug mode, maybe we can pass a configuration parameter that controls the print operation. Because I can think of use cases where someone would want to see the logs in release build also.

@pratikbaid3 pratikbaid3 added the enhancement New feature or request label Oct 18, 2024
@moritz157
Copy link
Author

Yes for sure. Thats what I proposed too actually.
My initial idea would simply be an optional boolean named something like logDebug. If it is null we only log if we are in debug mode and if it isn't null, we log if it is true. This would be a breaking change though, as we would disable logging in release builds for people who haven't added the configuration parameter yet.

An alternative would be passing an enum like the following:

enum SSELoggingConfig {
  always,
  debugOnly,
  never,
}

That way, the default value could be SSELoggingConfig.always, therefore keeping the current behaviour while providing convenient options too.

I don't know if this "breaking change" introduced by the first option would be critical enough to instead go with the admittedly kinda weird enum solution. I think it's questionable wether some people are currently relying on these debug messages in release builds.

Of course the simplest option of all would be to leave out the debugMode thing altogether and just add a boolean option defaulting to true.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
enhancement New feature or request
Projects
None yet
Development

No branches or pull requests

2 participants