Open dtoniolo opened 9 months ago
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()
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😄
Should be fixed now, let me know if you have any other observations or suggestions😄
PR #2323 added a feature that allows the creation of a
RollingFileAppender
that keeps only then
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 inprune_old_logs
at line 626 ofsrc/rolling.rs
, when an attempt at computingmax_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 toBuilder::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 anusize
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 ofn
altogether.Solution
This PR adds checks for the positiveness of
n
in the public-facing parts of the API, i.e. inBuilder::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
aNonZeroUsize
instead of anusize
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 oftracing_appender
. If you are favourable to this change, I could work on it and open a PR to have it fixed.