-
Notifications
You must be signed in to change notification settings - Fork 43
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
Make logging optional and update docs #189
Conversation
public var logger: Logger | ||
public var logger: Logger? |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
We can't make this change since this an API break. I think at this point there is no nice way to make this optional anymore sadly.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I see, I've reverted the changes of making it an optional, with only improved docs and readme examples remaining 👍
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
An alternative approach to make the logger in the initializers optional could be to chain the log handler from the passed in optional logger to the stored-property logger:
self.logger = Logger(logger.label ?? "ServiceLifecycle", factory: { _ in logger?.handler ?? SwiftLogNoOpLogHandler() })
Thanks for the change. Let's get this landed! |
Hello! 👋 This PR contains a few changes, notably:
README.md
to use the new initializer instead of the deprecated one.renamed
to the deprecated initializer.logging
parameter on the initializers optional, the motivation for this is in cases the user don't or can't do logging, they don't have to construct a logger and bootstrap the logging system withSwiftLogNoOpLogHandler
, so this code:becomes
and it also allows the user to do logging outside the ServiceGroup without necessarily doing logging within it.
If there is anything to improve, let me know! 😃
thanks dimi and gwynne for helping along the way and answering my questions :)