Change set_logger to panic on error

alexcrichton commented

2017-05-19 14:57:31 -05:00

(Migrated from github.com)

Almost all invocations of this want to panic (they're at the beginning of main) and this may also be able to remove the closure argument (according to @sfackler).

We should rename the current API to try_set_logger for suitable use during tests.

Almost all invocations of this want to panic (they're at the beginning of `main`) and this may also be able to remove the closure argument (according to @sfackler). We should rename the current API to `try_set_logger` for suitable use during tests.

alisha17 commented

2017-05-22 08:51:57 -05:00

(Migrated from github.com)

I am working on this.

dtolnay commented

2017-05-23 14:22:01 -05:00

(Migrated from github.com)

The same thing needs to happen for env_logger::init. There would be a try_init suitable for tests.

The same thing needs to happen for `env_logger::init`. There would be a `try_init` suitable for tests.

alisha17 commented

2017-05-26 03:44:52 -05:00

(Migrated from github.com)

Can you guide me a bit how should I go about resolving this? I don't have much experience with Rust, but would love to help.

alexcrichton commented

2017-05-28 12:38:02 -05:00

(Migrated from github.com)

@alisha17 sure yeah! So right now the set_logger API is a global registration point for all loggers. As a result you can't really call it twice because the first logger is already registered. For that reason it currently returns a Result indicating "your logger was installed" or "no, some other logger was installed first and you lost the race".

This issue, however, is that 100% of the time if you're initializing logging as part of fn main you want unwrap the result! In that sense we should just bake in the unwrap-related panic to the function itself. So we'll want to change the signature of set_logger to return nothing. Now at the same time, however, fn main isn't the only place that we initialize loggers. Tests frequently start all test cases with my_logger::init() as we're not sure which test runs first. In that case we don't care who wins the race for registration (everyone's registering the same logger) so you'll typically see drop(my_logger::init()) to ignore the results. In that sense we need to preserve today's functionality with a function like try_set_logger alongside set_logger that has the same semantics as today.

So overall I think we'll be changing the log crate to look like:


pub fn set_logger(logger: Box<Log>, filter: LogLevelFilter); 

pub fn try_set_logger<M>(make_logger: M) -> Result<(), SetLoggerError> 
    where M: FnOnce(MaxLogLevelFilter) -> Box<Log>;

Does that sound right to you @sfackler?

Also what @dtolnay was mentioning is that most users rarely actually call the set_logger or try_set_logger functions but rather a baked in initialization function in the logger they're using (e.g. env_logger::init). That just means we should, as part of this change, change the env_logger crate to have:

pub fn init(); // note the lack of Result
pub fn try_init() -> Result<(), SetLoggerError>; // same as `init` today

Lemme know if you've still got any more questions though!

@alisha17 sure yeah! So right now the `set_logger` API is a global registration point for all loggers. As a result you can't really call it twice because the first logger is already registered. For that reason it currently returns a `Result` indicating "your logger was installed" or "no, some other logger was installed first and you lost the race". This issue, however, is that 100% of the time if you're initializing logging as part of `fn main` you want unwrap the result! In that sense we should just bake in the `unwrap`-related panic to the function itself. So we'll want to change the signature of `set_logger` to return nothing. Now at the same time, however, `fn main` isn't the only place that we initialize loggers. Tests frequently start all test cases with `my_logger::init()` as we're not sure which test runs first. In that case we don't care who wins the race for registration (everyone's registering the same logger) so you'll typically see `drop(my_logger::init())` to ignore the results. In that sense we need to preserve today's functionality with a function like `try_set_logger` alongside `set_logger` that has the same semantics as today. So overall I think we'll be changing the `log` crate to look like: ```rust pub fn set_logger(logger: Box<Log>, filter: LogLevelFilter); pub fn try_set_logger<M>(make_logger: M) -> Result<(), SetLoggerError> where M: FnOnce(MaxLogLevelFilter) -> Box<Log>; ``` Does that sound right to you @sfackler? Also what @dtolnay was mentioning is that most users rarely actually call the `set_logger` or `try_set_logger` functions but rather a baked in initialization function in the logger they're using (e.g. `env_logger::init`). That just means we should, as part of this change, change the `env_logger` crate to have: ```rust pub fn init(); // note the lack of Result pub fn try_init() -> Result<(), SetLoggerError>; // same as `init` today ``` Lemme know if you've still got any more questions though!

alisha17 commented

2017-05-29 06:13:49 -05:00

(Migrated from github.com)

I guess the function set_logger already panics on error. It might have been fixed as a result of some other bug?
Although the renaming still remains to be done.

I guess the function set_logger already panics on error. It might have been fixed as a result of some other bug? Although the renaming still remains to be done.

dtolnay commented

2017-05-29 10:54:48 -05:00

(Migrated from github.com)

This set_logger function returns a SetLoggerError if there is an error. It does not panic.

[This set_logger function](https://github.com/rust-lang-nursery/log/blob/95dbcf7743f5d0385fb0df6e183f6b6c45215b9a/src/lib.rs#L823-L826) returns a SetLoggerError if there is an error. It does not panic.

alisha17 commented

2017-05-30 09:03:56 -05:00

(Migrated from github.com)

But how can I remove the closure argument in the set_logger function without changing the aesthetics of the function?

alexcrichton commented

2017-05-30 10:16:08 -05:00

(Migrated from github.com)

@alisha17 it'd probably look liek:

pub fn set_logger(logger: Box<Log>, filter: LogLevelFilter) {
    match try_set_logger(|max| { max.set(filter); logger) {
        Ok(()) => {}
        Err(_) => panic!("global logger is already initialized"),
    }
}

@alisha17 it'd probably look liek: ```rust pub fn set_logger(logger: Box<Log>, filter: LogLevelFilter) { match try_set_logger(|max| { max.set(filter); logger) { Ok(()) => {} Err(_) => panic!("global logger is already initialized"), } } ```

brson commented

2017-07-05 16:22:22 -05:00

(Migrated from github.com)

Fixed.

Rows
Columns

Change set_logger to panic on error #146