Failure case for TermLogger::new is unclear about simplelog.rs HOT 8 CLOSED

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.

from simplelog.rs.

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?

from simplelog.rs.

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.

from simplelog.rs.

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).

from simplelog.rs.

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.

from simplelog.rs.

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!

from simplelog.rs.

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.

from simplelog.rs.

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.

from simplelog.rs.