Am Sat, 12 Jul 2014 13:18:29 +0000
schrieb "Robert burner Schadek" <rburners@gmail.com>:

> > * Can the logger be influenced by command line flags /
> > environment
> >   variables? I guess not, but if it can it should be documented.
> I thought not writing about it makes it clear that you can not.
> 
Yes, the documentation is OK in that regard, that was more a question than a criticism about the docs ;-)


> > * Why does LogManager.defaultLogger return by ref instead of
> > just using
> >   a setter?
> Because I use the defaultLogger at quite a lot of places and treating it as an property just felt right.

Yes, it's perfectly fine as a property. But why not:

static @property @trusted Logger defaultLogger() //getter
{
    return ...;
}

static @property @trusted void defaultLogger(Logger value) //setter
{
    myLogger = value;
}

Usage of the property is exactly the same, but an explicit setter is more powerful and afaik we usually use explicit setters.

Overall looks good to me. Some points that haven't been mentioned so far in this review round:

 - Using a class with static members doesn't seem to be very idiomatic. It seems like the three member properties can simply be made global and everything should be fine - the "LogManager." prefix doesn't really add information. This has been mentioned in the last review round and it's not a very important point in this particular instance, but we should really make a decision here that will also decide how future modules go about this.

 - Personally, I really find additional verbose log levels useful. Currently there is only "trace". Previous proposals had a generic "verbose(N)" set of levels, but I think it's important for the proper interaction of multiple libraries to define a semantic set of verbose levels. A proposal, which has worked very well me (from low to high):

  trace:
     as now, used for low level tracing of program flow
  debugv:
    verbose debug messages, may flood the log
  debug:
    normal, low frequency debug messages, useful for the developer
  diagnostic:
    diagnostic output also potentially useful for the user
    this is what you typically would get with the -v command line switch

 - There is a "formatString" string mixin that includes a lot of if-else cases that seem to do exactly what formattedWrite would do anyway, is there something that it actually does in addition to that? Also, why is it a string mixin in contrast to a simple (template) function? It may be a matter of taste (but also of compile time/memory), but I'd almost always prefer a non-string-mixin solution.

 - Even if it may be more typing (or maybe not?), actually writing out the different signatures for each log level and the c/f suffixes would be very advantageous for the documentation and for code completion. It would also make the EBNF unnecessary, which I agree with Johannes looks a little scary. All of the functions could be based on the generic logl/loglf functions, so that there wouldn't be much redundancy.

 - I'm still really not sure if the "c" versions of the log functions pull their own weight. They seem to be in line with trivial convenience functions, which are generally discouraged in Phobos (with the same issues, such as additional cognitive load and bigger API size).

 - The functions error(), info(), fatal(), etc. don't follow the usual rule for functions to start with a verb. The question is if saving three characters over logError() is worth making the code more ambiguous for the outside reader (e.g. "does error() throw an exception? or set some internal error state?" "does fatal() terminate the process?")

On Sat, Jul 12, 2014 at 06:13:28PM +0200, Sönke Ludwig via Digitalmars-d wrote:
> Overall looks good to me. Some points that haven't been mentioned so far in this review round:
> 
>  - Using a class with static members doesn't seem to be very
> idiomatic. It seems like the three member properties can simply be
> made global and everything should be fine - the "LogManager." prefix
> doesn't really add information. This has been mentioned in the last
> review round and it's not a very important point in this particular
> instance, but we should really make a decision here that will also
> decide how future modules go about this.
[...]

+1. If something is a global, just call it a global. There's no need to be ashamed of that. Classes with only static members sound to me like forcing round pegs into square holes.


T

-- 
What doesn't kill me makes me stranger.

On 7/12/14, 4:04 AM, Dicebot wrote:
> On Saturday, 12 July 2014 at 03:22:10 UTC, Jesse Phillips wrote:
>> On Friday, 11 July 2014 at 14:39:09 UTC, David Nadlinger wrote:
>>> On Friday, 11 July 2014 at 14:36:34 UTC, Dicebot wrote:
>>>> Round of a formal review before proceeding to voting. Subject for
>>>> Phobos inclusion : http://wiki.dlang.org/Review/std.logger authored
>>>> by Robert Schadek.
>>>
>>> Is this for std.* or std.experimental.*?
>>>
>>> David
>>
>> I'm of the opinion we should review for std.* and vote for
>> std.experimental.*
>>
>> Prior to or during beta we vote to move into std.*
>
> Agreed.

That's nice. Doing laps in std.experimental for one release cycle can only improve form. Let's. -- Andrei

On 7/12/14, 10:19 AM, H. S. Teoh via Digitalmars-d wrote:
> On Sat, Jul 12, 2014 at 06:13:28PM +0200, Sönke Ludwig via Digitalmars-d wrote:
>> Overall looks good to me. Some points that haven't been mentioned so far in
>> this review round:
>>
>>   - Using a class with static members doesn't seem to be very
>> idiomatic. It seems like the three member properties can simply be
>> made global and everything should be fine - the "LogManager." prefix
>> doesn't really add information. This has been mentioned in the last
>> review round and it's not a very important point in this particular
>> instance, but we should really make a decision here that will also
>> decide how future modules go about this.
> [...]
>
> +1. If something is a global, just call it a global. There's no need to
> be ashamed of that. Classes with only static members sound to me like
> forcing round pegs into square holes.

I agree. This is not Java. That idiom must go. -- Andrei

>
>  ... I'd almost always prefer a non-string-mixin solution.
>
>  ... actually writing out the different signatures ...
>

+1

>From a maintenance/sanity standpoint, I'd much rather have a little bit of
duplication over one large if/else'd string mixin.  It is much easier to figure out exactly what is going on, and what the intent is, without having to pragma(msg) everything first.

On Sunday, 13 July 2014 at 07:14:50 UTC, Jeremy Powers via
Digitalmars-d wrote:
>>
>>  ... I'd almost always prefer a non-string-mixin solution.
>>
>>  ... actually writing out the different signatures ...
>>
>
> +1
>


I also think that the usage of mixins in std.log is an "unforced
error" and non-idiomatic D code.

A nice alternative is http://code.dlang.org/packages/log

On 2014-07-11 16:36, Dicebot wrote:
> Round of a formal review before proceeding to voting. Subject for Phobos
> inclusion : http://wiki.dlang.org/Review/std.logger authored by Robert
> Schadek.

* The free functions are not documented
* The API of the free functions look complicated and have cryptic names
* What's the EBNF for? Seems complicated as well

-- 
/Jacob Carlborg

On Saturday, 12 July 2014 at 14:04:00 UTC, Johannes Pfau wrote:

>
> The whole point of journald is that the user can define arbitrary
> key/value pairs. you're not limited to the special key/value pairs.
> http://0pointer.de/blog/projects/journal-submit.html
>
> Also giving log messages uuids is an important property of structured
> logging.

I will take a third look, I might have an idea solving the fixed size buffer and structured logging approach together.

On Saturday, 12 July 2014 at 21:23:28 UTC, Andrei Alexandrescu
wrote:

>
> I agree. This is not Java. That idiom must go. -- Andrei

I will fix this today