Review of Jose Armando Garcia Sancio's std.log

[Jose Armando Garcia]

On Tue, Feb 14, 2012 at 4:50 AM, Andrei Alexandrescu
<SeeWebsiteForEmail at erdani.org> wrote:
> On 2/13/12 9:50 AM, David Nadlinger wrote:
>>
>> Please post all feedback in this thread, and remember: Although
>> comprehensive reviews are obviously appreciated, short comments are very
>> welcome as well!
>
>
> Thanks Jose and David. I made a pass, here are a few thoughts:
>
> * "different verbose level" -> "different verbosity levels"
>
> * "functionality to disabled and enabled" -> "functionality to disable and
> enable"
>
> * "enviroment variables, and their meaning see" -> "enviroment variables and
> their meaning, see"
>
> * In code example: "Every nine" -> "Write every 9 passes"
>
> * In code example: unbraced try statement is odd. We should use our own
> conventions in examples.
>
> * In code example: plant an assert(false, "Never reached") after log!fatal.
>
Fixed all of the above comments.

> * first() and every() are quite useful. I'm thinking of complementing them
> with after(). "Once" is first(1).
>
We do have after(). It should be the second to last document block in
std_log.html. Yep, "Once is first(1) or just first() as the default
value for first is 1. The rest (every and after) don't have default
values because I couldn't find one that made sense to me.

> * "Descripton of the supported severities." -> "Description of supported
> severities." (notice the typo too)
>
Fixed.

> * vlog should take uint, not int.
>
There is a subtle reason why I decided to use int and not unit. I
suspect that most user will use non-negative values when using vlog.
They will probably start by using either vlog(0) and/or vlog(1).

To enable vlog(0) and vlog(1) they need to config.maxVerboseLevel(1),
config.maxVerboseLevel(2), etc. To enable vlog(0) and disable vlog(1)
they need to config.maxVerboseLevel(0). What if they want to disable
vlog(0)? They can't if we use uint. By using int instead of uint they
can always specify -1. Now, you can say that you can apply the same
logic to int.min. They will never be able to disable vlog(int.min). My
argument is that most user will not log using vlog(int.min) and the
ones that do are very familiar with the implementation and are aware
of the consequences. Thoughts?

> * When passing multiple parameters to log, they must be stringized
> automatically. So
>
> log!error("Log an ", to!string(Severity.error), " message!");
>
> becomes
>
> log!error("Log an ", Severity.error, " message!");
>
Fixed.

> * The examples right inside LogFilter don't mention it at all. I assume
> log!xyz has type LogFilter or something. That must be stated in writing.
>
Yes, but technically "template log" can either alias to LogFilter or
NoopLogFilter (used for compiled time disabling). But yes they have
structurally the same set of public methods.

Fixed.

> * log!info.when(first())("Only log this the first time in the loop") should
> really be log!info.when(first())("Only log this one time per application
> run").
>
It is actually per thread run. Change it to:
info.when(first())("Only log this one time per thread run")

> * No examples include durations.
>
Not sure it if help but a copied the unittest for duration. I should
be readable.

> * There might be a better name for Rich.
>
Suggestions?

> * assert(value == true); => assert(value);
>
Fixed.

Thanks!
-Jose
>
> Andrei