-
Notifications
You must be signed in to change notification settings - Fork 277
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
StdoutLogHandler should be public #51
Comments
Hi Allen, really cool that you're looking to adopt the API, and thanks for chiming in! As you notice, it is early days here, and not many implementations of this API exist yet. Our hope is that soon someone would implement a much better standard out logger that would support formatting etc. It is true though that the built in one provides a nice out of the box default... I think it sounds good on making it public, it seems to have been intended to be so; and it is accessible in any case for using it via "not configuring anything"; We could also address the docs comment on it while we're at it (perhaps something more clear that it's "boring" :-)), mentioning that it is intended to be a bare-minimum implementation and proper log handlers with formatting are expected to be external modules? We'll see what @weissi and @tomerd think, but sounds to me like a plan to have it publicly constructable, since we "ship it" and it is "to be used by users" anyway. |
I'd be okay with that, but we should name it |
Why stderr? https://12factor.net/logs |
Command line apps are pretty unusable if you log to
that'd break horribly if you log to |
I dug around a bit.
Maybe the 12-factor requirement of |
Yeah, I find it a bit odd that they require |
Yeah. I do think we should make |
@ianpartridge All real-world deployments will definitely have to take anything on |
Maybe we should ship |
yeah, could just have a |
👍 on shipping both, makes sense and does not widen the scope of the lib too much; this still counts as "bare minimum" etc. I think. Btw. re 12-factor apps... the rules are ok-ish... but in reality some of the hints are very weird (the configuration one + security springs to mind), so it's good as general inspiration but I don't think one should follow those when designing APIs like a bible of app design :-) |
Thanks for all the feedback here. Just wanted to say I can make |
@allenhumphreys I think we'd welcome that first PR :) |
@ianpartridge I went to commit and was following As part of my update I could:
|
Ah, thanks for letting us know. I think we should fix that in a separate PR - @weissi? |
Yes please :) |
|
Expose `StreamLogHandler` to the public which replaces the previously internal `StdoutLogHandlers`. Users can choose now the output stream, while `stdout` is the default. ### Motivation: As discussed on #51 and #60 ### Modifications: Rename `StdoutLogHandler` to `StreamLogHandler` and provide 2 factory methods `standardOutput(label:)` and `standardError(label:)` for using during bootstrapping. ### Result: Users who adopt other logging backends during `LoggingSystem` bootstrapping will still be able to use the default log handler to get very basic console output to their choice of `stdout` or `stderr`. ### Example: ``` // Log to both stderr and stdout for good measure LoggingSystem.bootstrap { MultiplexLogHandler([ FancyLoggingBackend(label: $0), StreamLogHandler.standardError(label: $0), StreamLogHandler.standardOutput(label: $0) ]) } ```
fixed by #61 |
Seems we missed to close this issue; Seems solved ( by #61 and see also https://github.com/apple/swift-log/blob/master/Sources/Logging/Logging.swift#L539-L549 ) |
Assigned to 1.0.1 milestone, and closed the 1.0.0 milestone as well since it seems to have been still open even tho we tagged 1.0.0. |
I'm looking at being an early adopter of SwiftLog because it looks good and I'm tired of solving the logging issue every time I start a new project. It occurs to me though, that
StdoutLogHandler
should bepublic
but it is marked asinternal
.The
StdoutLogHandler
is great because it provides out-of-the-box console logging to get up and moving fast and prevents the need to re-implement that, but being markedinternal
means that as soon as the use-case gets a little more interesting, I'd have to write my own console log handlers. The use case I'm referring to really just looks like this:I suppose
MultiplexLogHandler
could have some kind of configurability to implicitly create aStdoutLogHandler
in addition to whichever log handlers are passed in. This could be an alternative to exposingStdoutLogHandler
if that is truly the desire.It's also worth pointing out that
StdoutLogHandler
's initializer is explicitly marked public. So I'm wondering if this has been an oversight.I can open a PR to make a change if needed.
The text was updated successfully, but these errors were encountered: