(sorry for being a bit late, was distracted)

std.logger proposal by Robert Schadek enters voting period which will last two weeks starting from now.

Discussion thread : http://forum.dlang.org/post/zhvmkbahrqtgkptdlcvh@forum.dlang.org

This voting will be somewhat different from previous ones because it will be done with std.experimental in mind. Because of that please reply with a bit more structured votes:

1) Yes / No for inclusion into std.experimental

At this point please consider if module has functionality you want to see in standard library in general and if implementation is not fundamentally broken. This is a simple sanity check.

2) Yes / No for inclusion into Phobos in its current state

This is where you should summarize your concerns raised during review if there are any and make decision if existing API / architecture are promising enough to be set in stone via Phobos inclusion.

3) If you have answered "No" for (2) :  list of mandatory changes that are needed to make you vote "Yes"

4) Any additional comments for author.

Please separate (3) from (4) in some obvious fashion to make it possible for author to prioritize of feedback. Please use linked thread for discussions and only post vote + summary here.

Currently only answer for (1) affects the voting outcome. Other answers are necessary to eventually  prepare std.logger for second voting during beta period of some future release (for actual inclusion into Phobos).

If you have any comments / proposals about actual voting procedure or review process please create separate thread.

Go ahead ;)

Small foreword specifically about std.logger :

It is quite clear that something like logging system is very opinionated thing with API/design preferences highly based on actual use cases. I don't think that we will ever be able to come up with decisions that will satisfy everyone - some compromises are necessary. Because of that, please think twice before declaring certain features crucial or API unacceptable - reserve such judgement only for something that is really a blocker for your work and will likely result in using completely different logging solution as opposed to building on top of std.logger

And big thanks to Robert for being incredibly patient in pursuing inclusion of his proposal despite all the time it has taken!

On Tuesday, 29 July 2014 at 05:11:33 UTC, Dicebot wrote:
> 1) Yes / No for inclusion into std.experimental

If modules in std.experimental are completely free to make breaking changes, then my vote is a clear yes.

I'm uncertain if std.experimental can carry its own weight in the presence of dub, but I guess that's a discussion for another thread.

> 2) Yes / No for inclusion into Phobos in its current state

No.

IMO, both API and implementation are insufficient. Additionally, the "current state" is extremely volatile with sweeping API changes being made in the last two weeks or so.

However, review is on-going and at this rate, I'm hopeful it will be good enough by the next vote.

> 3) If you have answered "No" for (2) :  list of mandatory changes that are needed to make you vote "Yes"

a) The *API* must support minimal dynamic memory allocation for the normal execution path. However, theoretical *implementation* changes that reduce memory allocation are not a deal-breaker.

b) API must specify a strong stance on threading, whatever the form it takes.

c) This one might seem unnecessarily strict, but I think the fact that Logger is a class and not an interface smells of poor design. I might still be convinced that having it as a class is justified, but my current stance is that it must be an interface.

> 4) Any additional comments for author.

The author is aware of my gripes from Github comments and posts in the review thread.

On 29/07/2014 5:11 p.m., Dicebot wrote:
> (sorry for being a bit late, was distracted)
>
> std.logger proposal by Robert Schadek enters voting period which will
> last two weeks starting from now.
>
> Discussion thread :
> http://forum.dlang.org/post/zhvmkbahrqtgkptdlcvh@forum.dlang.org
>
> This voting will be somewhat different from previous ones because it
> will be done with std.experimental in mind. Because of that please reply
> with a bit more structured votes:
>
> 1) Yes / No for inclusion into std.experimental
>
> At this point please consider if module has functionality you want to
> see in standard library in general and if implementation is not
> fundamentally broken. This is a simple sanity check.

Yes,
I have not tested it, just for reference sake. But the code design looks fine. Nicely documented with unittests.

> 2) Yes / No for inclusion into Phobos in its current state
>
> This is where you should summarize your concerns raised during review if
> there are any and make decision if existing API / architecture are
> promising enough to be set in stone via Phobos inclusion.

Yes
But might be a good idea to add a Syslog and Windows event viewer logger(s) support.

> 3) If you have answered "No" for (2) :  list of mandatory changes that
> are needed to make you vote "Yes"
>
> 4) Any additional comments for author.

Okay 1) update sidebar for phobos docs (atleast I couldn't see link to modules in there).
2) for std.logger.core defaultLogger please please please add that at the top of the page of documentation. Or else the very ability to change the default logger could be lost. It took me a while to find it.

> Please separate (3) from (4) in some obvious fashion to make it possible
> for author to prioritize of feedback. Please use linked thread for
> discussions and only post vote + summary here.
>
> Currently only answer for (1) affects the voting outcome. Other answers
> are necessary to eventually  prepare std.logger for second voting during
> beta period of some future release (for actual inclusion into Phobos).
>
> If you have any comments / proposals about actual voting procedure or
> review process please create separate thread.
>
> Go ahead ;)

On 7/28/14, 10:11 PM, Dicebot wrote:
> (sorry for being a bit late, was distracted)
>
> std.logger proposal by Robert Schadek enters voting period which will
> last two weeks starting from now.
>
> Discussion thread :
> http://forum.dlang.org/post/zhvmkbahrqtgkptdlcvh@forum.dlang.org
>
> This voting will be somewhat different from previous ones because it
> will be done with std.experimental in mind. Because of that please reply
> with a bit more structured votes:

My vote is a qualified "yes" contingent upon fixes that I'll give detail on below. In the current form my vote is "no" seeing as the module makes a number of unforced tactical errors. Overall I think the goods are there, and are easy to put in acceptable form.

> 1) Yes / No for inclusion into std.experimental
>
> At this point please consider if module has functionality you want to
> see in standard library in general and if implementation is not
> fundamentally broken. This is a simple sanity check.

No in current form. Yes assuming the fixes below are implemented.

> 2) Yes / No for inclusion into Phobos in its current state
>
> This is where you should summarize your concerns raised during review if
> there are any and make decision if existing API / architecture are
> promising enough to be set in stone via Phobos inclusion.

No. I also think any new module should sit in std.experimental for one release cycle.

> 3) If you have answered "No" for (2) :  list of mandatory changes that
> are needed to make you vote "Yes"

Here's my list:

1. Minimal logging level must be selected statically in addition to the current dynamic selection. Static settings preclude dynamic settings. This is not negotiable.

2. All logging code must be rigorously eliminated if below the static logging level. More precisely, there must be a front-end optimization that eliminates all code dedicated to a "lazy" variable that's not used by a generic function. This would be a fantastic redeeming of the "lazy" keyword, which has been of marginal utility until now. The challenge here is cooperating with someone on the compiler team to make sure that front-end improvement gets implemented, and writing unit tests that make sure there's no regression later. This is not negotiable.

3. The suffix notations must be replaced with overloads. The only acceptable suffix is "f" for formatting. Everything else must be achieved via overloads with the help of template constraints. Robert's answer http://goo.gl/FehDVh suggests he didn't consider using template constraints. We can't let that slip become a feature of the library for millenia to come.

The overloads must be:

// just log stuff
log(T...)(lazy T stuff) if (!is(T[0] : const LogLevel));
// log with format
logf(S, T...)(lazy S fmt, lazy T stuff) if (isSomeString!Str);
// log conditional with format
logf(S, T...)(lazy bool cond, lazy S fmt, lazy T stuff) if (isSomeString!Str);

These three overloads should be repeated for all logging functions (info, trace etc). The functions don't evaluate their arguments if the respective log level is disabled.

The following functions will NOT be repeated for all logging functions:

// just log stuff at some level
log(T...)(LogLevel lvl, lazy T stuff) if (!is(T[0] : const LogLevel));
// log with format
logf(S, T...)(LogLevel lvl, lazy S fmt, lazy T stuff) if (isSomeString!Str);
// log conditional with format
logf(S, T...)(LogLevel lvl, lazy bool cond, lazy S fmt, lazy T stuff) if (isSomeString!Str);

These overloads always evaluate their first argument eagerly to determine the required logging level. Depending on it they may or may not evaluate their other arguments.

This is not negotiable.

4. Replace defaultLogger with theLog. "Logger" is a word, but one that means "lumberjack" so it doesn't have the appropriate semantics. The use is generally acceptable as a nice play on words and as a disambiguator between the verb "to log" and the noun "log". When we actually want to talk about the current log in an application, we should, however, call it "the log". This is negotiable.

5. I was hoping for a resolution on throttling. However, now I realize that conditional logging plus throttling functions that return true once in a while should work acceptably well. Higher-order functions taking lambdas that log would also be a nice possibility. So... no request here.

6. I'm still hoping for RefCounted as the storage for the class backend. I realize users can do their own management but log objects are unlikely to contain cycles and other liabilities for reference counting, and at some point if we want to use reference counting where appropriate we got to start somewhere with a few solid precedents. This is negotiable, but I plan to fight for it.

> 4) Any additional comments for author.

Don't let any of the above discourage you. This is great work and is already one foot in. Let's get this done and done. Don't forget - it's all about Deutsche Gründlichkeit!


Andrei

On Tuesday, 29 July 2014 at 06:09:25 UTC, Andrei Alexandrescu wrote:
> No in current form. Yes assuming the fixes below are implemented.

> ...

> No. I also think any new module should sit in std.experimental for one release cycle.

Clarification, just to be sure you got it right - right now we _only_ vote on inclusion into std.experimental, even if majority of voters will consider it Phobos-quality. Staging period of one release cycle is mandatory.

That said - can you explain a bit more why you think it can't be included in std.experimental? (== think that it is fundamentally broken to the point it can't be even suggested for experiments) Most issues listed seem to be more related to actual Phobos inclusion.

Dne 29.7.2014 7:11, Dicebot via Digitalmars-d napsal(a):
> (sorry for being a bit late, was distracted)
>
> std.logger proposal by Robert Schadek enters voting period which will
> last two weeks starting from now.
>
> Discussion thread :
> http://forum.dlang.org/post/zhvmkbahrqtgkptdlcvh@forum.dlang.org
>
> This voting will be somewhat different from previous ones because it
> will be done with std.experimental in mind. Because of that please reply
> with a bit more structured votes:
>
> 1) Yes / No for inclusion into std.experimental
>
> At this point please consider if module has functionality you want to
> see in standard library in general and if implementation is not
> fundamentally broken. This is a simple sanity check.

Yes.

The API is sane and the design has withstood a lot of relevant criticism as well as bikeshedding. Although, I do not really like log function suffixes and would prefer overloads.

> 2) Yes / No for inclusion into Phobos in its current state
>
> This is where you should summarize your concerns raised during review if
> there are any and make decision if existing API / architecture are
> promising enough to be set in stone via Phobos inclusion.

No.

There were a lot of changes during the review process. The module should IMO go to experimental to have some time to take care of all those small things as well as to get (hopefully) some larger adoption.

> 3) If you have answered "No" for (2) :  list of mandatory changes that
> are needed to make you vote "Yes"

For me it is the stay in std.experimental and seeing that nobody is pressing for another api changes (like Andrei is doing now).

> 4) Any additional comments for author.

Great work and some great patience you have displayed in the process.
Thank you!

On Tuesday, 29 July 2014 at 05:11:33 UTC, Dicebot wrote:
>
> 1) Yes / No for inclusion into std.experimental

Yes. It's ready for an official stamp.

> 2) Yes / No for inclusion into Phobos in its current state

No. Full advantage should be taken of the std.experimental time.

> 3) If you have answered "No" for (2) :  list of mandatory changes that are needed to make you vote "Yes"

Nothing new here, it's been covered by others. If I had to pick out something: Using overloads properly is a clear improvement step that must be taken for this to be considered phobos quality.

> 1) Yes / No for inclusion into std.experimental

Yes

> 2) Yes / No for inclusion into Phobos in its current state

No

> 3) If you have answered "No" for (2) :  list of mandatory changes that are needed to make you vote "Yes"

I can't say Yes until I've actually used it.

> 4) Any additional comments for author.

None for the author as I detailed my concerns in Github, though I should apologize for being very lax about participating (RL has been busy).  However, I wanted to address the suffix notation as I suggested it.  What I was going for was consistency with the write/writef method signatures to keep things consistent.  I felt it would be good to make the two similar since they do similar things.

My suggestion for conditional versions, logc and logcf, I believe are the ones causing some heartburn.  If you look at how write works, what does this mean?

    write(a < b, "some message");

Am I writing conditionally or am I writing out something like "truesome message"?  In phobos, it is the latter.  To have consistency, we can't simply make the first parameter be a condition as it would prevent us from doing something like this:

    log(canFind("foobar", "bar"));

Second, the way I look at it, you can read the methods like this:

    write - write
    writef - write formatted
    log - log
    logf - log formatted
    logc - log conditionally
    logcf - log conditionally and formatted

Having that english-like meaning I think will make it easier to recognize what's being done.

tl;dr I proposed having the log interface consistent with the write interface (from a parameter standpoint) and suggested the "c" suffix to make it clear that conditional logging is being performed vs. the first parameter being a boolean that's part of the log message.

maybe we should have made a vote for logc vs. log(bool) first

I think we gone full circle ulong.max times by now  ;-)