Change set_logger to panic on error #187

alisha17 · 2017-05-31T16:08:42Z

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

Fix to issue #146

dtolnay

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

mrhota

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

Thanks @alisha17!

mrhota · 2017-06-02T23:58:18Z

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 · 2017-06-03T00:05:02Z

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 · 2017-06-03T00:06:27Z

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 · 2017-06-03T00:19:57Z

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 · 2017-06-03T00:34:10Z

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 · 2017-06-03T00:34:34Z

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 · 2017-06-03T00:41:15Z

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 · 2017-06-03T02:44:26Z

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 · 2017-06-11T08:31:53Z

@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 · 2017-06-13T17:10:40Z

I agree with @mrhota.

alisha17 · 2017-06-15T00:57:00Z

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 · 2017-06-15T02:07:09Z

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

alisha17 · 2017-06-15T02:16:12Z

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

brson · 2017-06-15T02:17:21Z

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 · 2017-06-15T02:25:24Z

@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 · 2017-06-15T02:27:50Z

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

alisha17 · 2017-06-15T02:37:21Z

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

brson · 2017-06-15T21:41:18Z

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

alisha17 · 2017-06-17T00:09:10Z

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

brson · 2017-06-19T21:48:01Z

@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 · 2017-07-04T02:43:11Z

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 · 2017-07-05T17:14:10Z

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 · 2017-07-05T21:21:42Z

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

…est-0.11.11 Bump reqwest from 0.11.10 to 0.11.11

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

dtolnay reviewed Jun 2, 2017

View reviewed changes

mrhota reviewed Jun 3, 2017

View reviewed changes

brson reviewed Jun 3, 2017

View reviewed changes

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

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

brson force-pushed the setlogger branch from 2989faf to 088c5eb Compare June 15, 2017 02:01

brson mentioned this pull request Jun 15, 2017

set_logger wants to panic #199

Closed

brson force-pushed the setlogger branch from 5609045 to 088c5eb Compare June 15, 2017 02:21

set_logger panics on error

a1e0bca

alisha17 force-pushed the setlogger branch from e630108 to a1e0bca Compare June 22, 2017 00:53

alisha17 mentioned this pull request Jun 26, 2017

Rename set_logger_raw to try_set_logger_raw #204

Closed

brson merged commit f21e9e8 into rust-lang:master Jul 5, 2017

EFanZh pushed a commit to EFanZh/log that referenced this pull request Jul 23, 2023

Merge pull request rust-lang#187 from ryankurte/dependabot/cargo/reqw…

c042ccf

…est-0.11.11 Bump reqwest from 0.11.10 to 0.11.11

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Change set_logger to panic on error #187

Change set_logger to panic on error #187

alisha17 commented May 31, 2017

dtolnay left a comment

mrhota left a comment

mrhota Jun 2, 2017

mrhota Jun 3, 2017

mrhota Jun 3, 2017

mrhota Jun 3, 2017

mrhota Jun 3, 2017

mrhota Jun 3, 2017

mrhota Jun 3, 2017

brson Jun 3, 2017

mrhota commented Jun 11, 2017 •

edited

brson commented Jun 13, 2017

alisha17 commented Jun 15, 2017

brson commented Jun 15, 2017

alisha17 commented Jun 15, 2017

brson commented Jun 15, 2017

brson commented Jun 15, 2017 •

edited

brson commented Jun 15, 2017

alisha17 commented Jun 15, 2017 •

edited

brson commented Jun 15, 2017

alisha17 commented Jun 17, 2017

brson commented Jun 19, 2017

brson commented Jul 4, 2017

sfackler commented Jul 5, 2017

brson commented Jul 5, 2017

Change set_logger to panic on error #187

Change set_logger to panic on error #187

Conversation

alisha17 commented May 31, 2017

dtolnay left a comment

Choose a reason for hiding this comment

mrhota left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

mrhota commented Jun 11, 2017 • edited

brson commented Jun 13, 2017

alisha17 commented Jun 15, 2017

brson commented Jun 15, 2017

alisha17 commented Jun 15, 2017

brson commented Jun 15, 2017

brson commented Jun 15, 2017 • edited

brson commented Jun 15, 2017

alisha17 commented Jun 15, 2017 • edited

brson commented Jun 15, 2017

alisha17 commented Jun 17, 2017

brson commented Jun 19, 2017

brson commented Jul 4, 2017

sfackler commented Jul 5, 2017

brson commented Jul 5, 2017

mrhota commented Jun 11, 2017 •

edited

brson commented Jun 15, 2017 •

edited

alisha17 commented Jun 15, 2017 •

edited