Change set_logger to panic on error #187

alisha17 · May 31, 2017

There might be more things to fix. But just wanted to get feedback if I am on right track!

Fix to issue #146

dtolnay · Jun 2, 2017

dtolnay reviewed Jun 2, 2017

View changes

Thanks, looks like this is on the right track. Nicely done. Could you check on why the Travis build failed?

mrhota · Jun 3, 2017

mrhota reviewed Jun 3, 2017

View changes

OK, looks good to me, modulo a few nits and the fix required to pass the build

Thanks @alisha17!

mrhota · Jun 3, 2017

env/src/lib.rs

+            Box::new(logger)
+        })
+    }
+
    /// Initializes the global logger with an env logger.
    ///
    /// This should be called early in the execution of a Rust program, and the
    /// global logger may only be initialized once. Future initialization
    /// attempts will return an error.


Can you change this comment to say something like "Future initialization attempts will panic." instead of "return an error"?

mrhota · Jun 3, 2017

src/lib.rs

 /// Sets the global logger.
 ///
 /// The `make_logger` closure is passed a `MaxLevel` object, which the
 /// logger should use to keep the global maximum log level in sync with the
-/// highest log level that the logger will not ignore.
+/// highest log level that the logger wills not ignore.
 ///
 /// This function may only be called once in the lifetime of a program. Any log


You should mention that further calls to set_logger after the first will panic.

mrhota · Jun 3, 2017

src/lib.rs

 /// Sets the global logger.
 ///
 /// The `make_logger` closure is passed a `MaxLevel` object, which the
 /// logger should use to keep the global maximum log level in sync with the
-/// highest log level that the logger will not ignore.
+/// highest log level that the logger wills not ignore.


typo? should just stay as "logger will not ignore."

mrhota · Jun 3, 2017

src/lib.rs

@@ -818,11 +818,18 @@ pub fn max_level() -> LevelFilter {
    unsafe { mem::transmute(MAX_LOG_LEVEL_FILTER.load(Ordering::Relaxed)) }
 }

+#[cfg(feature = "use_std")]
+pub fn try_set_logger<M>(make_logger: M) -> Result<(), SetLoggerError>


TravisCI complained here about no documentation for this function. Since it's pub, I think it should have some.

Ping me if you want help coming up with some descriptive verbiage for the docs. Although, you can probably just paraphrase the docs from old set_logger

mrhota · Jun 3, 2017

tests/filters.rs

-                   Box::new(Logger(me))
-               })
-            .unwrap();
+    try_set_logger(|max| {


If the test only tests the try_ version, you can probably remove the set_logger def at line 18 and the set_logger import in line 9

mrhota · Jun 3, 2017

tests/max_level_features/main.rs

@@ -33,7 +40,7 @@ impl Log for Logger {

 fn main() {
    let mut a = None;
-    set_logger(|max| {
+    try_set_logger(|max| {


same reasoning applies here

mrhota · Jun 3, 2017

env/src/lib.rs

+
+    pub fn init(&mut self) {
+        let logger = self.build();
+        let filter = max_level.set(logger.filter());


max_level is cause of TravisCI failure, since it doesn't exist in this scope any more.

you can probably change to this-ish:

let logger = self.build(); let filter = logger.filter(); let logger = Box::new(logger); // <---- need to use let again to end previous binding's life, and begin another's log::set_logger(logger, filter);

warning: I didn't compile that ^^^ so check my work 😄

brson · Jun 3, 2017

brson reviewed Jun 3, 2017

View changes

brson · Jun 3, 2017

src/lib.rs

+    match try_set_logger(|max| { max.set(filter); logger }) {
+        Ok(()) => {}
+        Err(_) => panic!("global logger is already initialized"),
+    }
 }


The docs for this function will also need a "# Panics" section to describe the conditions when this function panics.

mrhota · Jun 11, 2017

@alisha17 TravisCI actually points to line 397, which is in the Logger::init function, not your new Builder::init function. That function expects Builder::init to return a Result, though, and now it just returns a ().

Not sure what the preferred fix is: maybe Logger should get the same treatment the Builder did (try_init and init)?

@brson probably knows better than me

brson · Jun 13, 2017

I agree with @mrhota.

alisha17 · Jun 15, 2017

The build is failing. The commit here and the commit in the build do not match. They are different. Is it possible that the test is not triggered for this commit?

brson · Jun 15, 2017

Indeed, both Travis and AppVeyor seem stunningly confused, and I can't seem to unconfuse them.

alisha17 · Jun 15, 2017

@brson Should I create an entirely new PR for this issue?

brson · Jun 15, 2017

No I don't think that will help. I pushed an empty PR to your branch to get the CI to test the new commit. If the tests succeed we can remove that commit again and merge.

brson · Jun 15, 2017

@alisha17 OK, I understand what's happening.

The CI on this repo seems to be set up to merge with master before testing. If you look at the commit it tested, one of the parents is your commit.

The errors it is reporting are legit. Something on master has invalidated one of the test cases when merged with your branch.

What you need to do is rebase your branch on top of the master branch, run the test suite again and fix that error, then force push back to your setlogger branch.

The rebase commands will look something like

git fetch origin
git rebase origin/master
... fix tests ...
git push alisha17 HEAD:setlogger -f

brson · Jun 15, 2017

Here's some documentation about git rebasing if you are not familiar: https://www.atlassian.com/git/tutorials/rewriting-history

alisha17 · Jun 15, 2017

Okay. Done with the rebasing.
When I run the test suite, all tests do pass. It shows no error(with current commit changes).

brson · Jun 15, 2017

@alisha17 You'll need to do a force push to your 'setlogger' branch so that it shows up on this PR.

alisha17 · Jun 17, 2017

@brson when I force push to my setlogger branch, it shows 'Everything up-to-date'.

brson · Jun 19, 2017

@alisha17 hm, if your local branch is rebased on upsteam master, then pushing to your setlogger branch shouldn't say everything is up to date. Your branch is on 088c5eb, and that is a descendant of a commit from May.

Here's what I see when I rebase your branch and run cargo test: https://gist.github.com/brson/a881e26af9b71b2d4cc981a72944dbb7

I get the same failure as travis does.

brson · Jul 4, 2017

This lgtm now. @sfackler are breaking changes like this ready to be merged in now, or do they need to wait until you are ready for a version bump?

sfackler · Jul 5, 2017

Let's just merge all of the breaking stuff into master - we'll cut an 0.3.x branch off of the last release to do the back compat stuff.

brson · Jul 5, 2017

OK, as far as I can tell this looks right, so I'm merging it. Thanks for your perseverance @alisha17!

alisha17 force-pushed the alisha17:setlogger branch from a7bfae7 to c32c1bf May 31, 2017

alisha17 force-pushed the alisha17:setlogger branch 3 times, most recently from 46c68e8 to d2025e4 Jun 9, 2017

alisha17 force-pushed the alisha17:setlogger branch 3 times, most recently from a6f61e5 to 088c5eb Jun 14, 2017

brson force-pushed the alisha17:setlogger branch from 2989faf to 088c5eb Jun 15, 2017

brson referenced this pull request Jun 15, 2017
Closed
set_logger wants to panic #199

brson force-pushed the alisha17:setlogger branch from 5609045 to 088c5eb Jun 15, 2017

set_logger panics on error

Loading status checks…

a1e0bca

alisha17 force-pushed the alisha17:setlogger branch from e630108 to a1e0bca Jun 22, 2017

alisha17 referenced this pull request Jun 26, 2017
Closed
Rename set_logger_raw to try_set_logger_raw #204

brson merged commit f21e9e8 into rust-lang-nursery:master Jul 5, 2017
2 checks passed

2 checks passed

continuous-integration/appveyor/pr AppVeyor build succeeded
Details

continuous-integration/travis-ci/pr The Travis CI build passed
Details

rust-lang-nursery/log

Join GitHub today

Change set_logger to panic on error #187

Conversation

alisha17 commented May 31, 2017

alisha17 force-pushed the alisha17:setlogger branch from a7bfae7 to c32c1bf May 31, 2017

dtolnay reviewed Jun 2, 2017 View changes

dtolnay left a comment

mrhota reviewed Jun 3, 2017 View changes

mrhota left a comment

This comment has been minimized.

This comment has been minimized.

This comment has been minimized.

This comment has been minimized.

This comment has been minimized.

This comment has been minimized.

This comment has been minimized.

brson reviewed Jun 3, 2017 View changes

This comment has been minimized.

alisha17 force-pushed the alisha17:setlogger branch 3 times, most recently from 46c68e8 to d2025e4 Jun 9, 2017

This comment has been minimized.

mrhota commented Jun 11, 2017 • edited

This comment has been minimized.

brson commented Jun 13, 2017

alisha17 force-pushed the alisha17:setlogger branch 3 times, most recently from a6f61e5 to 088c5eb Jun 14, 2017

This comment has been minimized.

alisha17 commented Jun 15, 2017

brson force-pushed the alisha17:setlogger branch from 2989faf to 088c5eb Jun 15, 2017

This comment has been minimized.

brson commented Jun 15, 2017

brson referenced this pull request Jun 15, 2017

This comment has been minimized.

alisha17 commented Jun 15, 2017

This comment has been minimized.

brson commented Jun 15, 2017

brson force-pushed the alisha17:setlogger branch from 5609045 to 088c5eb Jun 15, 2017

This comment has been minimized.

brson commented Jun 15, 2017 • edited

This comment has been minimized.

brson commented Jun 15, 2017

This comment has been minimized.

alisha17 commented Jun 15, 2017 • edited

This comment has been minimized.

brson commented Jun 15, 2017

This comment has been minimized.

alisha17 commented Jun 17, 2017

This comment has been minimized.

brson commented Jun 19, 2017

alisha17 force-pushed the alisha17:setlogger branch from e630108 to a1e0bca Jun 22, 2017

alisha17 referenced this pull request Jun 26, 2017

This comment has been minimized.

brson commented Jul 4, 2017

This comment has been minimized.

sfackler commented Jul 5, 2017

This comment has been minimized.

brson commented Jul 5, 2017

Hide details View details brson merged commit f21e9e8 into rust-lang-nursery:master Jul 5, 2017 2 checks passed

2 checks passed

alisha17 force-pushed the alisha17:setlogger branch from `a7bfae7` to `c32c1bf` May 31, 2017

dtolnay reviewed Jun 2, 2017

View changes

mrhota reviewed Jun 3, 2017

View changes

brson reviewed Jun 3, 2017

View changes

alisha17 force-pushed the alisha17:setlogger branch 3 times, most recently from `46c68e8` to `d2025e4` Jun 9, 2017

mrhota commented Jun 11, 2017 •

edited

alisha17 force-pushed the alisha17:setlogger branch 3 times, most recently from `a6f61e5` to `088c5eb` Jun 14, 2017

brson force-pushed the alisha17:setlogger branch from `2989faf` to `088c5eb` Jun 15, 2017

brson force-pushed the alisha17:setlogger branch from `5609045` to `088c5eb` Jun 15, 2017

brson commented Jun 15, 2017 •

edited

alisha17 commented Jun 15, 2017 •

edited

alisha17 force-pushed the alisha17:setlogger branch from `e630108` to `a1e0bca` Jun 22, 2017

brson merged commit `f21e9e8` into rust-lang-nursery:master Jul 5, 2017
2 checks passed