On Saturday, 12 July 2014 at 16:13:22 UTC, Sönke Ludwig 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.
>
>  - 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

the LogLevel enum has quite a lot of free number in between trace and info and so forth. In combination with a MultiLogger it is very easy to build verbose logging special to your needs.

>
>  - 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 about to change the codegen.

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

Anyone else?

>
>  - 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?")

if I change it back, people will argue that that is redundant and unintuitive. Than I will change it back again and the discussion starts again.

On Sunday, 13 July 2014 at 11:19:20 UTC, Dragos Carp wrote:
> 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.
>

But creating x , necessary, function with only slightly different names is also very non idiomatic D code. Anyway I'm working on that.

On Sunday, 13 July 2014 at 11:25:51 UTC, Jacob Carlborg wrote:
> 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 compiler does not create ddoc for mixined source

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

The names are quite easy if you look at the bnf.

On Sunday, 13 July 2014 at 11:57:26 UTC, Robert burner Schadek wrote:
>> - 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?")
>
> if I change it back, people will argue that that is redundant and unintuitive. Than I will change it back again and the discussion starts again.

I think those functions are used so often that can be worth an exception.

On 2014-07-13 14:01, Robert burner Schadek wrote:

> The compiler does not create ddoc for mixined source

Hasn't that been fixed recently? If not, then I don't think you should use mixins. We can't have undocumented functions.

> The names are quite easy if you look at the bnf.

If you don't use abbreviations there's no need for a EBNF.

-- 
/Jacob Carlborg

On 2014-07-13 13:57, Robert burner Schadek wrote:

> Anyone else?

I agree with Sönke.

> if I change it back, people will argue that that is redundant and
> unintuitive. Than I will change it back again and the discussion starts
> again.

"logError" is a lot more clear and descriptive. I think that's important. If people don't like that they can use an alias.

-- 
/Jacob Carlborg

On Sunday, 13 July 2014 at 12:18:40 UTC, Jacob Carlborg wrote:
> "logError" is a lot more clear and descriptive. I think that's important. If people don't like that they can use an alias.

It is effectively same as using static namespace class but C-style - mangling namespace into function name. `log.error` looks descriptive enough to me.

On Saturday, 12 July 2014 at 16:13:22 UTC, Sönke Ludwig wrote:
>  - 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?")

I use my own log module like this:
```
import log = util.log;

log.info(...);
log.debug(...);
// etc
```

On Sunday, 13 July 2014 at 12:41:59 UTC, sigod wrote:
> I use my own log module like this:
> ```
> import log = util.log;
>
> log.info(...);
> log.debug(...);
> // etc
> ```

Exactly. I think this should be popularized as default style via std.logger documentation.

>
> But creating x , necessary, function with only slightly different names is also very non idiomatic D code. Anyway I'm working on that.

For the conditional variants, wouldn't be enough an overload with
the condition on the first position?