Skip to content

std.log: add scoped logging struct - #6039

Merged
andrewrk merged 3 commits into
ziglang:masterfrom
heidezomp:std-log-scoped
Aug 12, 2020
Merged

std.log: add scoped logging struct#6039
andrewrk merged 3 commits into
ziglang:masterfrom
heidezomp:std-log-scoped

Conversation

@heidezomp

@heidezompheidezomp commented Aug 12, 2020

Copy link
Copy Markdown
Contributor

This is an implementation of @daurnimator's proposal: #5943 (comment)

  • Add a std.log.scoped function that returns a scoped logging struct
  • Add a std.log.default struct that logs using the .default scope
  • Update the module documentation and example to explain the difference between using explicit scopes, using a scoped logging namespace, and using the default namespace

Note that I named the function "scoped" instead of "scope" so as not to clash with the scope parameter that is used everywhere; I didn't feel comfortable simply renaming the scope parameter to something like "s" or "log_scope" everywhere.

Questions for review

  1. Are you okay with changing the function name to "scoped" instead of "scope" as mentioned in the proposal, or would you rather rename the "scope" parameter?
  2. The intended usage pattern of const log = std.log.scoped(.main); log.err("oops", .{}); can be used everywhere except in the root source file, if that file overrides the log implementation (since it would result in a naming conflict). Is this a problem? Maybe the log implementation could be renamed to "logger" instead?
  3. Bikeshedding: name for the default scope? The ones I've seen mentioned are .app, .default, .main and .root.

Closes#5943

 * Add a std.log.scoped function that returns a scoped logging struct
* Add a std.log.default struct that logs using the .default scope
Implementation of daurnimator's proposal:
#5943 (comment)
Note that I named the function "scoped" instead of "scope" so as not to
clash with the scope parameter that is used everywhere; this seemed a
better solution to me than renaming the scope parameter to "s" or
"log_scope" or the like.
 * Add short documentation to std.log.scoped and std.log.default
* Update the module documentation and example to explain the difference
between using explicit scopes, using a scoped logging namespace, and
using the default namespace
Since the logger implementation can be overridden, the messages might
not be logged to stderr at all.
@heidezomp
heidezomp marked this pull request as ready for review August 12, 2020 14:00
@andrewrk

Copy link
Copy Markdown
Member

The intended usage pattern of const log = std.log.scoped(.main); log.err("oops", .{}); can be used everywhere except in the root source file, if that file overrides the log implementation (since it would result in a naming conflict). Is this a problem? Maybe the log implementation could be renamed to "logger" instead?

I don't see this as a problem, especially since it only exists when you explicitly override the log function (only doing const log = std.log.default for example does not use pub so it is fine).

The simple use case (not overriding log) is fine. The advanced use case is also fine since advanced users know how to deal with the name conflict.

Are you okay with changing the function name to "scoped" instead of "scope" as mentioned in the proposal, or would you rather rename the "scope" parameter?

👍

Bikeshedding: name for the default scope? The ones I've seen mentioned are .app, .default, .main and .root.

.default is fine for now. We can revisit this when solving the use case of switching the default log scope for third party packages.


Nice work!

@andrewrk
andrewrk merged commit 30db5b1 into ziglang:masterAug 12, 2020
@nmichaels

nmichaels commented Aug 12, 2020

Copy link
Copy Markdown
Contributor

Shouldn't this have done something like

pubconstemerg=default.emerg;
pubconstalert=default.alert;
pubconstcrit=default.crit;
pubconsterr=default.err;
pubconstwarn=default.warn;
pubconstinfo=default.info;
pubconstdebug=default.debug;

and deleted the old ones and moved their docstrings over, so it would be used as

std.log.err("error message: {}", .{ohnoes});
instead of
std.log.default.err("error message: {}", .{ohnoes});
?

@heidezomp
heidezomp deleted the std-log-scoped branch August 12, 2020 22:59
@heidezomp

heidezomp commented Aug 12, 2020

Copy link
Copy Markdown
ContributorAuthor

@nmichaels The way I read the proposal, using a scoped logging namespace is a feature in addition to the current logging interface. Personally I would be fine with restructuring it in the way you mention, but I don't know whether people want to explicitly keep the current interface as well.

Edit: Since this has been merged already, do you want me to do a follow-up PR?

@andrewrk

Copy link
Copy Markdown
Member

I support @nmichaels follow up proposal

@nmichaels

Copy link
Copy Markdown
Contributor

It is a breaking API change, but I think it's an improvement. A quick grep of the standard library didn't turn up anything obvious that depends on the current API, so it's probably not as much work as I originally assumed it would be.

@heidezomp

Copy link
Copy Markdown
ContributorAuthor

Cool, I'll send a follow-up PR tomorrow then. Thank you both for the helpful reviews! 😄

Sign up for freeto 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.

remove the "scope" parameter from std.log functions; introduce "std.log.scoped" functions

3 participants

@heidezomp@andrewrk@nmichaels