On Fri, Mar 2, 2012 at 12:56 AM, Brad Roberts <bra...@puremagic.com> wrote:
> On 2/13/2012 7:50 AM, David Nadlinger wrote:
>> There are several modules in the review queue right now, and to get things
>> going, I have volunteered to manage the
>> review of Jose's std.log proposal. Barring any objections, the review period
>> starts now and ends in three weeks, on
>> March 6th, followed by a week of voting.
>>
>> ---
>> Code:
>> https://github.com/jsancio/phobos/commit/d114420e0791c704f6899d81a0293cbd3cc8e6f5
>> Docs: http://jsancio.github.com/phobos/phobos/std_log.html
>>
>> Known remaining issues:
>> - Proof-reading of the docs is required.
>> - Not yet fully tested on Windows.
>>
>> Depends on: https://github.com/D-Programming-Language/druntime/pull/141
>> (will be part of 2.058)
>> ---
>>
>> Earlier drafts of this library were discussed last year, just search the NG
>> and ML archives for "std.log".
>>
>> I think getting this right is vitally important so that we can avoid an
>> abundance of partly incompatible logging
>> libraries like in Java. Thus, I'd warmly encourage everyone to actively try
>> out the module or compare it with any
>> logging solution you might already be using in your project.
>>
>> Please post all feedback in this thread, and remember: Although
>> comprehensive reviews are obviously appreciated, short
>> comments are very welcome as well!
>>
>> David
>
> My 2 cents from a fairly quick scan of the docs:
>
Thanks for looking at the documentation.
> 1) I'm of the opinion that it should be possible to strip all log code from
> an app without changing it's behavior.
> Having log levels that change execution flow is evil. It's it the same class
> of bad practices as assert expressions
> having side effects, imho.
>
I think this depends on your point of view. One way to look at
critical("critical") is as a replacement to enforce() that logs. We
could extends critical in a way similar to enforce where you can
specify the exception that should be thrown. David, would this help
with the issues you express earlier?
> 2) Apps over a certain size (that tends to not be all that big, a few 10's of
> thousands of lines) tend to start to need
> module based logging. This proposal includes a set of log levels that have
> no concept of modularity and another
> separate set that do. The distinction seems arbitrary and limiting.
>
This distinction is not a limitation of the logger API. It is a
limitation of the configuration API. In the future we can add the
option to enable error, warning, info at the module level without
breaking existing code.
> 3) The conditional stuff seems cute, but I can't recall ever wanting it in
> anything I've done before.
>
> 4) I don't see an obvious facility for changing log parameters at runtime,
> unless the intent is to build a paramstring
> array as if it came from command line parameters and calling parseCommandLine
> again. use case: long running application
> that has a configuration api or notices a config file has been updated or
> whatever. Fairly common behavior.
>
If you want to change the log level at runtime you can just:
std.log.config.minSeverity = Severity.info;
No need to call parseCommandLine. Everything that parseCommandLine
does can be implemented by a client user. Really parseCommandLine
doesn't need to be part of the Configuration class. It is just there
for grouping and documentation.
> 5) The logger severity symbols part should allow more than single character
> tokens. IMHO, the default should be the
> full name of the severity, not just the first character.
>
My motivation for using one char is that it is easier to parse by both
computers and humans. It is also makes the framework faster since it
writes less bytes. I think we can extends this and fix this once we
have custom line formatted.
> 6) Is the log system thread safe? I see that it's at least thread aware, but
> what guarantees are about log entry atomicity?
>
Yes, it is. It is probably to conservative in this regard. I need to
go back and do a lot performances improvements.
> 7) The logger setter docs says it's an error to change the logger after a log
> message has been emitted. That's going to
> hurt. It's not at all uncommon for an app to want to log some set of errors
> before it's potentially had enough time to
> execute the code that configures the logger. use case: reading a config file
> to get the logger configuration, but can't
> process the config file properly.
>
This was brought up before. There is no technical reason for this and
I will remove it.
Thanks!
> Later,
> Brad
