February 14, 2012
On Tue, Feb 14, 2012 at 4:50 AM, Andrei Alexandrescu <SeeWebsiteForEmail@erdani.org> wrote:
> On 2/13/12 9:50 AM, David Nadlinger wrote:
>>
>> Please post all feedback in this thread, and remember: Although comprehensive reviews are obviously appreciated, short comments are very welcome as well!
>
>
> Thanks Jose and David. I made a pass, here are a few thoughts:
>
> * "different verbose level" -> "different verbosity levels"
>
> * "functionality to disabled and enabled" -> "functionality to disable and enable"
>
> * "enviroment variables, and their meaning see" -> "enviroment variables and their meaning, see"
>
> * In code example: "Every nine" -> "Write every 9 passes"
>
> * In code example: unbraced try statement is odd. We should use our own conventions in examples.
>
> * In code example: plant an assert(false, "Never reached") after log!fatal.
>
Fixed all of the above comments.

> * first() and every() are quite useful. I'm thinking of complementing them
> with after(). "Once" is first(1).
>
We do have after(). It should be the second to last document block in
std_log.html. Yep, "Once is first(1) or just first() as the default
value for first is 1. The rest (every and after) don't have default
values because I couldn't find one that made sense to me.

> * "Descripton of the supported severities." -> "Description of supported severities." (notice the typo too)
>
Fixed.

> * vlog should take uint, not int.
>
There is a subtle reason why I decided to use int and not unit. I
suspect that most user will use non-negative values when using vlog.
They will probably start by using either vlog(0) and/or vlog(1).

To enable vlog(0) and vlog(1) they need to config.maxVerboseLevel(1),
config.maxVerboseLevel(2), etc. To enable vlog(0) and disable vlog(1)
they need to config.maxVerboseLevel(0). What if they want to disable
vlog(0)? They can't if we use uint. By using int instead of uint they
can always specify -1. Now, you can say that you can apply the same
logic to int.min. They will never be able to disable vlog(int.min). My
argument is that most user will not log using vlog(int.min) and the
ones that do are very familiar with the implementation and are aware
of the consequences. Thoughts?

> * When passing multiple parameters to log, they must be stringized automatically. So
>
> log!error("Log an ", to!string(Severity.error), " message!");
>
> becomes
>
> log!error("Log an ", Severity.error, " message!");
>
Fixed.

> * The examples right inside LogFilter don't mention it at all. I assume log!xyz has type LogFilter or something. That must be stated in writing.
>
Yes, but technically "template log" can either alias to LogFilter or NoopLogFilter (used for compiled time disabling). But yes they have structurally the same set of public methods.

Fixed.

> * log!info.when(first())("Only log this the first time in the loop") should
> really be log!info.when(first())("Only log this one time per application
> run").
>
It is actually per thread run. Change it to:
info.when(first())("Only log this one time per thread run")

> * No examples include durations.
>
Not sure it if help but a copied the unittest for duration. I should be readable.

> * There might be a better name for Rich.
>
Suggestions?

> * assert(value == true); => assert(value);
>
Fixed.

Thanks!
-Jose
>
> Andrei
February 14, 2012
On Mon, Feb 13, 2012 at 1:50 PM, David Nadlinger <see@klickverbot.at> 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

Updated the document an implementation to reflect every suggest for I replied to with the exception of providing a format parameter for thread name. Should have that ready later today. Let me know if I missed anything. API changes:

1. Dropped log!info("message"), etc. Use info("message), etc.
2. opCall now alias to format. Ie. info("Format %s message", Severity.info).
    this means that to concatenate strings you need to
info.write("Hello ", "world");

>
> 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
February 14, 2012
On Tue, Feb 14, 2012 at 5:44 AM, Jacob Carlborg <doob@me.com> wrote:
> On 2012-02-13 22:17, Sönke Ludwig wrote:
>>
>> Log levels "debug" and maybe also "trace" would be useful, but I see
>> that vlog(n)() is meant for that purpose. I would just prefer explicit
>> names instead of just numbers.
>>
>> Is there a compelling reason why formatted logging is not the default? I find that most logging calls in practice use formatted output, and the only overhead would be searching once through the format string in the case of format placeholders.
>>
>> A predefined logger for OutputDebugString on Windows would be useful - or maybe it could be used instead of stdout at least for non-console applications.
>>
>> One kind of log writer that I have in my code is one that outputs a nicely formatted HTML file with built-in JavaScript to be able to filter messages by priority or module. Maybe this is too much for a standard library implementation though.
>
>
> It would be nice to be able to plug in different formatters.
>
What do you mean by formatter? For example the default Logger (FileLogger) allow you to specify the format line see FileLogger.Configuration.lineFormat

If you want to do the HTML stuff Sönke mentioned then you need to inherit Logger and overwrite config.logger. Mind you that the Logger API should be view almost like a journaling API and HTML is a document so the differences need to be reconciled in the implementation of Logger.

Thanks,
-Jose

>
>> Support for multiple log writers can be useful (e.g. logging to a file + logging to stdout or to a log control inside of the running application). Of course, one can also simply write a "MultiDispatchLogger"...
>>
>> A format option to log the thread name instead of just the ID.
>
>
>
> --
> /Jacob Carlborg
February 14, 2012
On Tue, Feb 14, 2012 at 8:42 AM, jdrewsen <jdrewsen@nospam.com> wrote:
> On Tuesday, 14 February 2012 at 02:28:11 UTC, Jose Armando Garcia wrote:
>>
>> On Mon, Feb 13, 2012 at 6:44 PM, jdrewsen <jdrewsen@nospam.com> wrote:
>>>
>>> A first quick observation:
>>>
>>> I vote for a debug severity level. Then make that default to the template parameter for log:
>>>
>>> template log(Severity severity = Severity.debug)
>>>
>>> That would make it nice for good old print debugging.
>>>
>>> log("This is a dbg message");
>>>
>>
>> I like the idea of having a default. Not sure about adding debug. What are you trying to do with default that log!info and vlog(#) doesn't let you do?
>
>
> As Sean mentioned the vlog function may be the one I want. Maybe it is okey not to have a debug severity but then a default on the vlog level parameter would be nice. That would make quick debug prints a tad simpler
>
If we do set a default what should it be? It is not clear to me what value we should pick so if you have any suggestions let me know.

> vlog("This is a dbg message");
>
> I know this is a small thing... but to prevent death by a 1000 cuts.
>
> /Jonas
>
>
>
February 14, 2012
On Tue, Feb 14, 2012 at 11:14 AM, Jacob Carlborg <doob@me.com> wrote:
> On 2012-02-13 16:50, 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
>
>
> Is it possible to log to other locations, i.e. to the standard location for the given platform.
>
What do you mean by location? If you mean file system directory then:
@property string logDirectory(string logDirectory);
const const @property string logDirectory();
    Specifies the directory where log files are created.

    The default value for this property is the value in the
environment variable LOGDIR. If LOGDIR is not set, then TEST_TMPDIR is
used. If TEST_TMPDIR is not set, then it logs to the current
directory.

If you mean file name, you have 'fileNamePrefixes' and 'fileNameExtension'.

If you mean syslog then for now the user needs to implement this but I foresee us implementing this in phobos in the near future.

Thanks,
-Jose

> --
> /Jacob Carlborg
February 14, 2012
On Mon, Feb 13, 2012 at 1:50 PM, David Nadlinger <see@klickverbot.at> 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

It is better if we use this: https://github.com/D-Programming-Language/phobos/pull/432/files That link will stay up to date as I make changes to the code.

> 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
February 14, 2012
On Tuesday, 14 February 2012 at 16:21:42 UTC, Jose Armando Garcia wrote:
> On Tue, Feb 14, 2012 at 8:42 AM, jdrewsen <jdrewsen@nospam.com> wrote:
>> On Tuesday, 14 February 2012 at 02:28:11 UTC, Jose Armando Garcia wrote:
>>>
>>> On Mon, Feb 13, 2012 at 6:44 PM, jdrewsen <jdrewsen@nospam.com> wrote:
>>>>
>>>> A first quick observation:
>>>>
>>>> I vote for a debug severity level. Then make that default to the template
>>>> parameter for log:
>>>>
>>>> template log(Severity severity = Severity.debug)
>>>>
>>>> That would make it nice for good old print debugging.
>>>>
>>>> log("This is a dbg message");
>>>>
>>>
>>> I like the idea of having a default. Not sure about adding debug. What
>>> are you trying to do with default that log!info and vlog(#) doesn't
>>> let you do?
>>
>>
>> As Sean mentioned the vlog function may be the one I want. Maybe it is okey
>> not to have a debug severity but then a default on the vlog level parameter
>> would be nice. That would make quick debug prints a tad simpler
>>
> If we do set a default what should it be? It is not clear to me what
> value we should pick so if you have any suggestions let me know.

IMO a default severity level is not a good idea, not explicit to begin with.
As i suggested on another reply, getting rid of the instantiations solve it.

We lose nothing and gain a common keyword. I used to have severity levels for my logging library in c++. As soon as i got the C++0x options i thrashed them all.

Now instead of:

txt(error) << "this is: " << it;

i just got:

error("this is: ", it);

Win win for every aspect of it. And got rid of the keyword "txt".
February 14, 2012
On Tuesday, 14 February 2012 at 17:48:06 UTC, so wrote:
> On Tuesday, 14 February 2012 at 16:21:42 UTC, Jose Armando Garcia wrote:
>> On Tue, Feb 14, 2012 at 8:42 AM, jdrewsen <jdrewsen@nospam.com> wrote:
>>> On Tuesday, 14 February 2012 at 02:28:11 UTC, Jose Armando Garcia wrote:
>>>>
>>>> On Mon, Feb 13, 2012 at 6:44 PM, jdrewsen <jdrewsen@nospam.com> wrote:
>>>>>
>>>>> A first quick observation:
>>>>>
>>>>> I vote for a debug severity level. Then make that default to the template
>>>>> parameter for log:
>>>>>
>>>>> template log(Severity severity = Severity.debug)
>>>>>
>>>>> That would make it nice for good old print debugging.
>>>>>
>>>>> log("This is a dbg message");
>>>>>
>>>>
>>>> I like the idea of having a default. Not sure about adding debug. What
>>>> are you trying to do with default that log!info and vlog(#) doesn't
>>>> let you do?
>>>
>>>
>>> As Sean mentioned the vlog function may be the one I want. Maybe it is okey
>>> not to have a debug severity but then a default on the vlog level parameter
>>> would be nice. That would make quick debug prints a tad simpler
>>>
>> If we do set a default what should it be? It is not clear to me what
>> value we should pick so if you have any suggestions let me know.
>
> IMO a default severity level is not a good idea, not explicit to begin with.
> As i suggested on another reply, getting rid of the instantiations solve it.
>
> We lose nothing and gain a common keyword. I used to have severity levels for my logging library in c++. As soon as i got the C++0x options i thrashed them all.
>
> Now instead of:
>
> txt(error) << "this is: " << it;
>
> i just got:
>
> error("this is: ", it);
>
> Win win for every aspect of it. And got rid of the keyword "txt".

I also like your proposal better. In the end I just want a simple to type function call syntax or it will be tempting to fallback to writeln(...).

/Jonas

February 14, 2012
On Tuesday, 14 February 2012 at 16:12:57 UTC, Jose Armando Garcia wrote:
> On Mon, Feb 13, 2012 at 1:50 PM, David Nadlinger <see@klickverbot.at> 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
>
> Updated the document an implementation to reflect every suggest for I
> replied to with the exception of providing a format parameter for
> thread name. Should have that ready later today. Let me know if I
> missed anything. API changes:
>
> 1. Dropped log!info("message"), etc. Use info("message), etc.
> 2. opCall now alias to format. Ie. info("Format %s message", Severity.info).
>   this means that to concatenate strings you need to
> info.write("Hello ", "world");
>

In the introduction text the references to Configuration etc. should be made into links.

/Jonas
February 14, 2012
On Tue, Feb 14, 2012 at 4:44 PM, jdrewsen <jdrewsen@nospam.com> wrote:
> On Tuesday, 14 February 2012 at 16:12:57 UTC, Jose Armando Garcia wrote:
>>
>> On Mon, Feb 13, 2012 at 1:50 PM, David Nadlinger <see@klickverbot.at> 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
>>
>>
>> Updated the document an implementation to reflect every suggest for I replied to with the exception of providing a format parameter for thread name. Should have that ready later today. Let me know if I missed anything. API changes:
>>
>> 1. Dropped log!info("message"), etc. Use info("message), etc.
>> 2. opCall now alias to format. Ie. info("Format %s message",
>> Severity.info).
>>  this means that to concatenate strings you need to
>> info.write("Hello ", "world");
>>
>
> In the introduction text the references to Configuration etc. should be made into links.
>
I would love to do that. How do you do that with ddoc?

> /Jonas