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

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

Closed
andrewrk opened this issue Jul 28, 2020 · 9 comments · Fixed by #6039
Closed
Labels
accepted This proposal is planned. breaking Implementing this issue could cause existing code to no longer compile or have different behavior. contributor friendly This issue is limited in scope and/or knowledge of Zig internals. proposal This issue suggests modifications. If it also has the "accepted" label then it is planned. standard library This issue involves writing Zig code for the standard library.
Milestone

Comments

@andrewrk
Copy link
Member

andrewrk commented Jul 28, 2020

Accepted Proposal


In effort to make logging more mainstream and ergonomic, this is a proposal to simplify the logging API, yet still provide the advanced functionality.

It's difficult to justify the "scope" parameter, for example, in Hello World. Here I propose to do this:

diff --git a/lib/std/log.zig b/lib/std/log.zig
index d8bcba38c..3bd62e3c0 100644
--- a/lib/std/log.zig
+++ b/lib/std/log.zig
@@ -90,6 +90,8 @@ pub const default_level: Level = switch (builtin.mode) {
     .ReleaseSmall => .emerg,
 };
 
+const default_scope = .main;
+
 /// The current log level. This is set to root.log_level if present, otherwise
 /// log.default_level.
 pub const level: Level = if (@hasDecl(root, "log_level"))
@@ -117,7 +119,7 @@ fn log(
 
 /// Log an emergency message to stderr. This log level is intended to be used
 /// for conditions that cannot be handled and is usually followed by a panic.
-pub fn emerg(
+pub fn emergScope(
     comptime scope: @Type(.EnumLiteral),
     comptime format: []const u8,
     args: anytype,
@@ -126,9 +128,16 @@ pub fn emerg(
     log(.emerg, scope, format, args);
 }
 
+/// Log an emergency message to stderr. This log level is intended to be used
+/// for conditions that cannot be handled and is usually followed by a panic.
+pub fn emerg(comptime format: []const u8, args: anytype) void {
+    @setCold(true);
+    log(.emerg, default_scope, format, args);
+}
+
 /// Log an alert message to stderr. This log level is intended to be used for
 /// conditions that should be corrected immediately (e.g. database corruption).
-pub fn alert(
+pub fn alertScope(
     comptime scope: @Type(.EnumLiteral),
     comptime format: []const u8,
     args: anytype,
@@ -137,10 +146,17 @@ pub fn alert(
     log(.alert, scope, format, args);
 }
 
+/// Log an alert message to stderr. This log level is intended to be used for
+/// conditions that should be corrected immediately (e.g. database corruption).
+pub fn alert(comptime format: []const u8, args: anytype) void {
+    @setCold(true);
+    log(.alert, default_scope, format, args);
+}
+
 /// Log a critical message to stderr. This log level is intended to be used
 /// when a bug has been detected or something has gone wrong and it will have
 /// an effect on the operation of the program.
-pub fn crit(
+pub fn critScope(
     comptime scope: @Type(.EnumLiteral),
     comptime format: []const u8,
     args: anytype,
@@ -149,9 +165,17 @@ pub fn crit(
     log(.crit, scope, format, args);
 }
 
+/// Log a critical message to stderr. This log level is intended to be used
+/// when a bug has been detected or something has gone wrong and it will have
+/// an effect on the operation of the program.
+pub fn crit(comptime format: []const u8, args: anytype) void {
+    @setCold(true);
+    log(.crit, default_scope, format, args);
+}
+
 /// Log an error message to stderr. This log level is intended to be used when
 /// a bug has been detected or something has gone wrong but it is recoverable.
-pub fn err(
+pub fn errScope(
     comptime scope: @Type(.EnumLiteral),
     comptime format: []const u8,
     args: anytype,
@@ -160,10 +184,17 @@ pub fn err(
     log(.err, scope, format, args);
 }
 
+/// Log an error message to stderr. This log level is intended to be used when
+/// a bug has been detected or something has gone wrong but it is recoverable.
+pub fn err(comptime format: []const u8, args: anytype) void {
+    @setCold(true);
+    log(.err, default_scope, format, args);
+}
+
 /// Log a warning message to stderr. This log level is intended to be used if
 /// it is uncertain whether something has gone wrong or not, but the
 /// circumstances would be worth investigating.
-pub fn warn(
+pub fn warnScope(
     comptime scope: @Type(.EnumLiteral),
     comptime format: []const u8,
     args: anytype,
@@ -171,9 +202,16 @@ pub fn warn(
     log(.warn, scope, format, args);
 }
 
+/// Log a warning message to stderr. This log level is intended to be used if
+/// it is uncertain whether something has gone wrong or not, but the
+/// circumstances would be worth investigating.
+pub fn warn(comptime format: []const u8, args: anytype) void {
+    log(.warn, default_scope, format, args);
+}
+
 /// Log a notice message to stderr. This log level is intended to be used for
 /// non-error but significant conditions.
-pub fn notice(
+pub fn noticeScope(
     comptime scope: @Type(.EnumLiteral),
     comptime format: []const u8,
     args: anytype,
@@ -181,9 +219,15 @@ pub fn notice(
     log(.notice, scope, format, args);
 }
 
+/// Log a notice message to stderr. This log level is intended to be used for
+/// non-error but significant conditions.
+pub fn notice(comptime format: []const u8, args: anytype) void {
+    log(.notice, default_scope, format, args);
+}
+
 /// Log an info message to stderr. This log level is intended to be used for
 /// general messages about the state of the program.
-pub fn info(
+pub fn infoScope(
     comptime scope: @Type(.EnumLiteral),
     comptime format: []const u8,
     args: anytype,
@@ -191,12 +235,24 @@ pub fn info(
     log(.info, scope, format, args);
 }
 
+/// Log an info message to stderr. This log level is intended to be used for
+/// general messages about the state of the program.
+pub fn info(comptime format: []const u8, args: anytype) void {
+    log(.info, default_scope, format, args);
+}
+
 /// Log a debug message to stderr. This log level is intended to be used for
 /// messages which are only useful for debugging.
-pub fn debug(
+pub fn debugScope(
     comptime scope: @Type(.EnumLiteral),
     comptime format: []const u8,
     args: anytype,
 ) void {
     log(.debug, scope, format, args);
 }
+
+/// Log a debug message to stderr. This log level is intended to be used for
+/// messages which are only useful for debugging.
+pub fn debug(comptime format: []const u8, args: anytype) void {
+    log(.debug, default_scope, format, args);
+}

Hello World becomes:

const std = @import("std");

pub fn main() void {
    std.log.info("application start: {}", .{"Hello, World!"});
}

A potential future improvement would be if we had package-local configurations and therefore could swap out the default log scope per-package. So packages that didn't need multiple scopes would be able to use the simpler log functions, and then get assigned an optional scope override by the application that used the package.

cc @ifreund

@andrewrk andrewrk added standard library This issue involves writing Zig code for the standard library. proposal This issue suggests modifications. If it also has the "accepted" label then it is planned. labels Jul 28, 2020
@andrewrk andrewrk added this to the 0.7.0 milestone Jul 28, 2020
@andrewrk andrewrk changed the title remove the "scope" parameter from std.log functions; introduce std.logScope remove the "scope" parameter from std.log functions; introduce "std.log.fooScope" functions Jul 28, 2020
@ifreund
Copy link
Member

ifreund commented Jul 28, 2020

I think that in its current state this proposal makes the API more ergonomic for trivial use-cases like hello world but less ergonomic for larger codebases and all libraries (which should always use a custom scope to allow for filtering). Having some kind of package-local configuration would make this a win for libraries however, and being able to override the config of imported packages would put the power in the hands of the library user as well as solving the issue of potential scope conflicts.

I'd actually argue that hello world should just use std.debug.print (which is what is currently done in the docs for master). Both scope and logging levels are irrelevant to the goal of just printing a message. However if the goal is to introduce users to std.log early, using it in hello world would make sense and I can definitely see how the scope parameter would be confusing to those new to the language. I also think it makes sense to have zig init-exe use std.log as the goal is not to simply print a message but to provide a stub that will be expanded into an actual program.

@deingithub
Copy link
Contributor

deingithub commented Jul 28, 2020

I agree with @ifreund on the scope (heh) of std.log here, but if this proposal is accepted anyway, I think it should be done the other way around: preserve the short std.log.info for the "correct", more desirable logging with scopes, and add new symbols like std.log.infoNoScope/std.log.infoDefaultScope for the "quick and dirty" approach.

@nmichaels
Copy link
Contributor

nmichaels commented Jul 28, 2020

I think the case where someone's going to want to call log functions with different scopes in the same file will be pretty unusual. As such, maybe the right thing to do is to provide the default scope on the const log = line. We could keep the scope variants for the unusual case where a log message needs different scope from the rest of the file.

As for init-exe, this isn't a library so it makes sense to pick a reasonable default scope like ".root" or ".app".

@andrewrk
Copy link
Member Author

less ergonomic for larger codebases and all libraries

how about a _s suffix instead of Scope suffix? related: #1097

@ifreund
Copy link
Member

ifreund commented Jul 28, 2020

how about a _s suffix instead of Scope suffix? related: #1097

I don't think log.warn_s() is terribly clear. What if we put the scoped versions in std.log.scoped or similar? Then one could do either of the following depending on the type of project:

const log = std.log;
log.err("oops", .{}); // Uses the default_scope
const log = std.log.scoped;
log.err(.main, "oops", .{});

@andrewrk
Copy link
Member Author

andrewrk commented Aug 5, 2020

Ooh, I like that scoped namespace idea. Brilliant.

@andrewrk andrewrk added accepted This proposal is planned. breaking Implementing this issue could cause existing code to no longer compile or have different behavior. contributor friendly This issue is limited in scope and/or knowledge of Zig internals. labels Aug 5, 2020
@andrewrk andrewrk changed the title remove the "scope" parameter from std.log functions; introduce "std.log.fooScope" functions remove the "scope" parameter from std.log functions; introduce "std.log.scoped" functions Aug 5, 2020
@daurnimator
Copy link
Collaborator

I would prefer a function in std.log that closes over the scope:

pub fn scope(comptime s: any) type {
    return struct {
        pub fn err(comptime fmt: []const u8, args: any) void {
            return log(.err, s, fmt, any);
        }
        // and other levels
    };
}
pub const default = scope(.default);

to be used as:

const log = std.log.scope(.main);
log.err("oops", .{});

or otherwise:

const log = std.log.default;
log.err("oops", .{});

@pixelherodev
Copy link
Contributor

pixelherodev commented Aug 5, 2020 via email

@ifreund
Copy link
Member

ifreund commented Aug 5, 2020

I agree, @daurnimator's suggestion is better. Even if you want to mix scopes in the same file you can do something like

const log = std.log;
log.scope(.main).err("oops", .{});
log.scope(.linker).crit("oof", .{});

heidezomp added a commit to heidezomp/zig that referenced this issue Aug 12, 2020
 * 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:
ziglang#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.
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
accepted This proposal is planned. breaking Implementing this issue could cause existing code to no longer compile or have different behavior. contributor friendly This issue is limited in scope and/or knowledge of Zig internals. proposal This issue suggests modifications. If it also has the "accepted" label then it is planned. standard library This issue involves writing Zig code for the standard library.
Projects
None yet
Development

Successfully merging a pull request may close this issue.

6 participants