Adds a check in tracing_appender

[dtoniolo]

PR #2323 added a feature that allows the creation of a RollingFileAppender that keeps only the n most recent log files on each rotation.

I find this feature to be very useful and have been using it in my code every since tracing_appender 0.2.3 came out. I opened this PR with the intention of introducing a couple of minor improvements on top of the changes brought by PR #2323.

Motivation

The current version of the code leaves open the possibility for n to be 0, a value that makes no sense and that causes a crash in prune_old_logs at line 626 of src/rolling.rs, when an attempt at computing max_files - 1 is made. It seems to me that this is not a major pitfall for the user, as it is easy to avoid passing 0 to Builder::max_log_files, but I believe that the code should still perform some basic check.

Moreover, when I first read the documentation for this method I was confused by the fact that n was modelled with an usize and that no mention of the special value of 0 was made. In particular, I did not understand if 0 was just a forbidden value, a special value with an ad-hoc meaning, or if I was misinterpreting the meaning of n altogether.

Solution

This PR adds checks for the positiveness of n in the public-facing parts of the API, i.e. in Builder::max_log_files. This method panics if the check fails, thus anticipating the error that would occur during rotation at the source of the error, making the error easier to debug.

It also adds a line in the documentation that makes it clear that 0 is a forbidden value.

Future Improvements

Making n a NonZeroUsize instead of an usize would make its semantics clearer and reduce the likelihood of an error, both for the users and in the internal parts of the code. It would, however, constitute a breaking change, so it could be reserved e.g. for v0.3 of tracing_appender. If you are favourable to this change, I could work on it and open a PR to have it fixed.

[kaffarell]

Ohh, NonZeroUsize is a breaking change because rust doesn't coerce a usize to a NonNull* value even if it is known at compile time, that sucks :(

So if NonZeroUsize is introduced the user will have to change

.max_log_files(5)

to

.max_log_files(5.try_into().unwrap())

which isn't that beautiful, but I think its slightly better (at least better than panicking? [0] ).

I guess this change is still fine for v1.0.x...
Although I would use assert instead of an if-statement + panic! call :)
Something like: assert!(n != 0, "value cannot be 0");

[0]: In terms that the library is panicking, in the second example above the application code is panicking at the .unwrap()

[dtoniolo]

It didn't occur to me I could use an assert, which does indeed seem a little bit more elegant. I'll change that tomorrow😄

[dtoniolo]

Should be fixed now, let me know if you have any other observations or suggestions😄

[kaffarell]

LGTM!

[hawkw]

This change looks good to me, thanks for the PR! I suggested some very small style changes I'd like to make, and then I'm going to go ahead and merge this.

I agree that NonZeroUsize could be worth considering for a breaking change release in the future, but the benefit of enforcing non-zero at the type level ought to be weighed against the cost of introducing more complexity for the caller, as @kaffarell pointed out...