Failure case for TermLogger::new is unclear

[Horgix]

Context

The examples (both in the documentation and in the README) for TermLogger initialization with TermLogger::new suggest to .unwrap() the result directly :

TermLogger::new(LevelFilter::Warn, Config::default(), TerminalMode::Mixed).unwrap(),

Issue

I recently had the surprise to see it failing. In short, it was in a CI environment where the TERM environment variable was not set, resulting in TermInfo::new failing, and thus term::stdout() and term::stderr() too. It took a while to debug mainly because TermLogger::new() was failing without any hint on why it was failing, may it be in the documentation or in the returned information.

Note: I don't use TermLogger::init() but I guess it is impacted by the same issue.

Fix

What would be the best fix there? Would it make sense to have TermLogger::new return a Result instead of an Option so it could convey more information in the returned Err ?

If this would be too much of a breaking change, I'll submit a PR on the documentation explaining why TermLogger::new() could return None.

[Drakulix]

Yes, you are right, this should be an actual error instead of an Option.

Thanks for your PR, but I would rather see an implementation using a Result instead because I will probably need to bump the version anyway (again) to finally fix #19.

[Horgix]

Hey @Drakulix , Glad to read that you're open to a breaking change to improve things 🙂

I'll submit a PR in the upcoming days to change the retun of TermLogger::new from Option to Result and have nice Err messages if that's OK for you. Could you tell me if TermLogger::init is concerned too and if I should take a look at it at the same time?

[Drakulix]

I'll submit a PR in the upcoming days to change the retun of TermLogger::new from Option to Result and have nice Err messages if that's OK for you. Could you tell me if TermLogger::init is concerned too and if I should take a look at it at the same time?

Thanks, that sounds absolutely OK. 😄

TermLogger::init should also be updated. It already returns an Error because setting the logger globally might fail, e.g. if another one is already set, but the Term-Variant should be updated to contain the Error returned by TermLogger::new.

[ryankurte]

i just discovered this too, in a systemd unit TERM is undefined so the logger fails to start.
it'd be neat to document this (or alternatively default to xterm or something rather than failing out).

[Drakulix]

i just discovered this too, in a systemd unit TERM is undefined so the logger fails to start.
it'd be neat to document this (or alternatively default to xterm or something rather than failing out).

Allowing TermLogger to fail is absolutely reasonable and will not be changed. You are supposed to handle errors and silently "fixing" this by hacking together a non-existing TERM variable is not the solution. systemd does not set TERM deliberately and you should instead fall back to SimpleLogger if TermLogger fails, which will work in the circumstances.

[Firstyear]

I think the concern is more about the failure case not being documented. I see a few possible ways to deal with this

• Don't fail (but you have made it clear that there are failure cases)
• Document the failure cases
• Document the best practice of falling back to SimpleLogger in your readme/top level docs to handle these events.

Certainly the "surprise" failure in CI/docker/systemd would catch people out who want a logger to stdout, so advertising the best practice to handle this situation would be really helpful.

Thanks!

[ryankurte]

yeah that's p much it ^_^

Allowing TermLogger to fail is absolutely reasonable and will not be changed. You are supposed to handle errors

the dx here is that you setup a TermLogger following the example in the README or api, then when you try and daemonise it either faults out with a non-useful error or silently fails and your logs disappear depending on whether you let _ = or .unwrap().

silently "fixing" this by hacking together a non-existing TERM variable is not the solution.

you should instead fall back to SimpleLogger if TermLogger fails, which will work in the circumstances.

so, documenting this failure and how to mitigate it in the API docs / example, and improving the error would improve the situation, but as an alternative could TermLogger fall back to SimpleLogger if no term variable is set? that seems like a more predictable behaviour, and means that every user of TermLogger doesn't have to handle the Error::NoTermSet case.

[Drakulix]

Upon discovering, that term does not provide an error, but just an Option as well, I decided to keep it. term does not give us further information and I do not want to create an error case based on reading it's source code.

Instead, I have decided to merge @Horgix original PR #41 and expand it with some further examples in 70b09e2.