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.
>
> Code:
>     https://github.com/D-Programming-Language/phobos/pull/1500
> Documentation:
>     http://burner.github.io/phobos/phobos-prerelease/std_logger_core.html
>     http://burner.github.io/phobos/phobos-prerelease/std_logger_stdiologger.html
>     http://burner.github.io/phobos/phobos-prerelease/std_logger_filelogger.html
>     http://burner.github.io/phobos/phobos-prerelease/std_logger_multilogger.html
>     http://burner.github.io/phobos/phobos-prerelease/std_logger_nulllogger.html
>     http://burner.github.io/phobos/phobos-prerelease/std_logger_templatelogger.html
> DUB package:
>     http://code.dlang.org/packages/logger
>
> Previous discussion thread:
>     http://forum.dlang.org/post/mailman.313.1377180809.1719.digitalmars-d@puremagic.com
>
> ======================================================================
>
> Summary of changes since last discussion (by Robert):
>
>
> * logger is now a package of multiple files
> * a lot more documentation
> * log and logf now behave as their write and writef counterparts
> * for logging with an explicit LogLevel call logl loglf
> * for logging with an explicit condition call logc logcf
> * for logging with an explicit LogLevel and explicit condition call
> loglc loglcf
> * the log function with an explicit LogLevel like info, warning, ... can
> be extended into c, f or cf and therefore require a condition and/or are
> printf functions
> * unittest have been updated
> * dub package for easy testing
>
> ======================================================================
>
> This is one of long sanding Phobos candidates Robert has put some great effort into. As far as I know it is already used in several D projects and it is a good moment to make it official.
>
> Review goals, as usual : verify that API is robust enough to build more complicated logging systems on top of it, verify Phobos style compatibility, check if provided documentation is complete and friendly enough.
>
> Base for review process is defined by http://wiki.dlang.org/Review/Process

Mistype at std.logger.core.Tracer: "Tracer generates trace calls to the passed logger wenn the Tracer struc" wenn should be "when".

The API is well done. The only thing I want to have in the package is a delayed logger: delayed logger wraps another logger, accumulates messages and writes to inner logger by explicit call.

That is useful while testing. Major part of diagnostic information is needed only when test is failed:
```
unittest
{
    auto logger = new DelayedLogger(myCustomLogger);
    scope(failure) logger.flush;

    // some activity that can throw
}
```

Some static analysis warnings:

std/logger/core.d(808:18)[warn]: Parameter cond is never used
std/logger/core.d(1069:10)[warn]: Variable ll is never used
std/logger/core.d(1106:14)[warn]: Variable tracer is never used
std/logger/core.d(1161:9)[warn]: Variable nLineNumber is never used
std/logger/core.d(1263:12)[warn]: Variable name is never used
std/logger/core.d(1300:12)[warn]: Variable name is never used
std/logger/core.d(1323:10)[warn]: Variable readLine is never used

Several of the functions in the API are missing "Params:" sections.

Le 11/07/2014 16:36, Dicebot a écrit :
> Round of a formal review before proceeding to voting. Subject for Phobos
> inclusion : http://wiki.dlang.org/Review/std.logger authored by Robert
> Schadek.
>
> Code:
>      https://github.com/D-Programming-Language/phobos/pull/1500
> Documentation:
> http://burner.github.io/phobos/phobos-prerelease/std_logger_core.html
> http://burner.github.io/phobos/phobos-prerelease/std_logger_stdiologger.html
>
> http://burner.github.io/phobos/phobos-prerelease/std_logger_filelogger.html
> http://burner.github.io/phobos/phobos-prerelease/std_logger_multilogger.html
>
> http://burner.github.io/phobos/phobos-prerelease/std_logger_nulllogger.html
> http://burner.github.io/phobos/phobos-prerelease/std_logger_templatelogger.html
>
> DUB package:
>      http://code.dlang.org/packages/logger
>
> Previous discussion thread:
> http://forum.dlang.org/post/mailman.313.1377180809.1719.digitalmars-d@puremagic.com
>
>
> ======================================================================
>
> Summary of changes since last discussion (by Robert):
>
>
> * logger is now a package of multiple files
> * a lot more documentation
> * log and logf now behave as their write and writef counterparts
> * for logging with an explicit LogLevel call logl loglf
> * for logging with an explicit condition call logc logcf
> * for logging with an explicit LogLevel and explicit condition call
> loglc loglcf
> * the log function with an explicit LogLevel like info, warning, ... can
> be extended into c, f or cf and therefore require a condition and/or are
> printf functions
> * unittest have been updated
> * dub package for easy testing
>
> ======================================================================
>
> This is one of long sanding Phobos candidates Robert has put some great
> effort into. As far as I know it is already used in several D projects
> and it is a good moment to make it official.
>
> Review goals, as usual : verify that API is robust enough to build more
> complicated logging systems on top of it, verify Phobos style
> compatibility, check if provided documentation is complete and friendly
> enough.
>
> Base for review process is defined by http://wiki.dlang.org/Review/Process

In the documentation you tell about informations added by the module (filename, linenumber,...) but you don't put sample of the resulting output.
IMO it's important to provide a standard format of the output then D's IDE would be able to parse it to create links to the code or colorize it,...

Does the output contain a message counter? This help to distinguish message duplicates, on some console when prints are fast we don't see lines added if their are all the same.

Is it too late for an integration in std.expirimental with dmd 2.66?

https://github.com/burner/logger/pull/8

I've fixed some of the typos pointed out in this thread.

currently the import reads std.logger; Actually I don't care, as long as
I get more comments and tester.

OT to David: after rereading the std.experimental thread, I'm currently
collecting ideas for a DIP about it.

On Friday, 11 July 2014 at 15:20:51 UTC, Johannes Pfau wrote:
>
> writeLogHeader would receive all data as usual, except for the message
> string. Then after calling writeLogHeader, the (formatted) message
> string will be passed in any number of calls to put.
>
> I used put here to make this output-range compatible. This way it plugs
> directly into formattedWrite.
>
>
> An logger can still receive the complete string by appending the chunks
> received in put but we might need some 'finish' function to signal the
> end of the message. I guess not all loggers can fit into this
> interface, so we should try to make this optional. But simple loggers
> (file, console) which write the header first and the message last could
> work without any dynamic memory allocation. (formattedWrite probably
> uses an fixed-size buffer on the stack, but that's fine)

The api for none printf like logging has changed into something
like
write. So put properly needs to become a template. Any I'm not
sure if
there is a nice way around the template/inheritance problematic.
Other
than options one and two.

On Friday, 11 July 2014 at 18:02:58 UTC, Marc Schütz wrote:
> Some logging backends (e.g. systemd journal) support structured logging. Should support for this be included (as a subclass, presumably)?

Well, the idea behind std.logger is that I can not and should not even try to give you implementations for all your backend needs. It will just fall short and you will tell me that is it bad and should not go into phobos. And even if I manage it is going to be curl all over again.

So subclass Logger, implement writeLogMsg to your needs and be happy.

Yes, Tracer is doomed. It is gone go. scope(exit) is way smarter

On Friday, 11 July 2014 at 20:57:06 UTC, Xavier Bigand wrote:
>
> In the documentation you tell about informations added by the module (filename, linenumber,...) but you don't put sample of the resulting output.
I will add one.
> IMO it's important to provide a standard format of the output then D's IDE would be able to parse it to create links to the code or colorize it,...

I see your point, but people will change the output, we have multiple IDE's who do stuff different, so creating a framework for that is properly out of scope. But in the templatelogger module there is a function I would currently consider the default. I'll think about adding a very simple regex parser that returns a LoggerPayload.
>
> Does the output contain a message counter? This help to distinguish message duplicates, on some console when prints are fast we don't see lines added if their are all the same.
It does not. Adding that to the LoggerPayload will be no problem.
>
> Is it too late for an integration in std.expirimental with dmd 2.66?
ask Andrew, but I'm 99.99999% sure it is to late

On Friday, 11 July 2014 at 21:09:10 UTC, Brian Schott wrote:
> https://github.com/burner/logger/pull/8
>
> I've fixed some of the typos pointed out in this thread.

I like your style, thank you, "Talk is cheap ..."