On 10/14/2013 02:39 PM, Sönke Ludwig wrote:

Am 14.10.2013 13:39, schrieb Dicebot:

As `std.logger` is still marked as "work in progress" this thread is
less formal that typical pre-voting review. Goal is to provide as much
input about desirable `std.logger` functionality and current state and
let module author to use this information for preparing proposal for
actual review / voting.

Lets unleash the forces of constructive destruction.

===== Meta =====

Code: https://github.com/D-Programming-Language/phobos/pull/1500/files
Docs: http://burner.github.io/phobos/phobos-prerelease/std_logger.html

First announcement / discussion thread :
http://forum.dlang.org/post/mailman.313.1377180809.1719.digitalmars-d@puremagic.com

Sorry in advance for the long list of issues. I think the general approach is fine, but over the years I've grown some preferences for this stuff. Generally, I think we should make sure that such a module is flexible enough to fulfill most people's needs, or it will probably fail the widespread adoption that is desired to actually improve interoperability.

- LogLevel: enum values should start lower case according to the
Phobos conventions.

will be fixed

- The static methods in LogManager should be made global and the class
be removed. It's not for objects so it shouldn't be a class.

LogManager also stores the global log level. Sure I can make another static global function storing this log level, but I would like to keep them together as they belong together IMO.

- For me this logger is completely worthless without any debug log
   levels. The last std.log entry had at least anonymous verbosity
   levels, but I'd prefer something like I did in vibe.d [1], where
   each level has a defined role. This should especially improve the
   situation when multiple libraries are involved.

Logger.log(LogLevel.(d|D)ebug, "Your message");

- Similarly, I've never felt the need for conditional logging, but
without it being lazy evaluated what's the use for it, actually?

The conditional logging part is totally transparent.

- I really think there should be shortcuts for the different log
levels. Typing out "LogLevel.xxx" each time looks tedious for
something that is used in so many places.

One could argue that writting logger.logDebug("...") is more tedious than writing,
logger.logLevel = LogLevel.xxx;
logger.log("...");
logger.log("...");
...

This has been argued in the last logger discussion to some extend and it looked to me like this is the mostly preferred version.

- There should be some kind of MultiLogger so that multiple log
destinations can be configured (e.g. console + file). Or, instead of
a single default logger, there could be a list of loggers.

there is one default logger. I will create a MultiLogger, good point. I'm currently sure how to store the multi logger (LL or Array or ... )

- Logger.logMessage: Exchanging the bunch of parameters with a single
   struct that is passed by reference makes the API much more
   flexible/future-proof and is more efficient when the data needs to
   be passed on to other functions.

good point

- "shared" - probably best to leave this out until we have a verified
design for that - but in theory the loggers should probably be
shared.

my thoughts exactly

- On the same topic, if I'm right and the default logger is stored as
__gshared, it should be documented that Loggers need to be
thread-safe.

It is not stored __gshared, but If, you're right.

- GC allocations for each log message _must_ be avoided at all costs.
   Using formattedWrite() instead of format() on either a temporary
   buffer that is reused, or, better, on some kind of output range
   interface of the Logger would solve this.

This was proposed in the last thread. A fixed size buffer would scream bufferoverflow, a dynamic buffer not but both would raise the question of thread safety.

- A note on DDOC comments: The first paragraph of a doc comment is
   treated as a short description according to the DDOC spec. It should
   be kept to around a single sentence, followed by a more detailed
   paragraph. While this doesn't matter much with the current HTML doc
   layout, the short description is used in the single-page
   documentation inside of overview tables [2].

Will be fixed

Awesome comments, thanks