July 11, 2014 Review: std.logger | ||||
---|---|---|---|---|
| ||||
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 |
July 11, 2014 Re: Review: std.logger | ||||
---|---|---|---|---|
| ||||
Posted in reply to Dicebot | As usual, this review round will last for 2 weeks unless someone asks for delay. Please share link to this thread via twitter, reddit, G+ and whatever else may be used out there. |
July 11, 2014 Re: Review: std.logger | ||||
---|---|---|---|---|
| ||||
Posted in reply to Dicebot | 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.
Is this for std.* or std.experimental.*?
David
|
July 11, 2014 Re: Review: std.logger | ||||
---|---|---|---|---|
| ||||
Posted in reply to David Nadlinger | On Friday, 11 July 2014 at 14:39:09 UTC, David Nadlinger wrote:
> 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.
>
> Is this for std.* or std.experimental.*?
>
> David
Deciding this is subject of this review/voting iteration too - it is mostly matter of API stability, how much of a trust reviewers are ready to put into existing API.
Personally I believe that for something like logging library stabilization period of one release cycle in std.experimental is desirable because wider usage is very likely to result in breaking change suggestions.
|
July 11, 2014 Re: Review: std.logger | ||||
---|---|---|---|---|
| ||||
Posted in reply to Dicebot | The API seems nice and simple while providing enough flexibility, and I think it would work quite well for my purposes. Some comments: API: Not clear what the difference between globalLogLevel and a log level on each individual logger is. Does the global one only affect the global logger? What happens if you change the globalLogLevel and the logLevel on the logger set as the global one? Documentation: Logger Constructor: "The fatal handler will throw, and Error if a log call is made with a LogLevel LogLevel.fatal." - Should be "will throw an error if". LogManager class: "It also handels the defaultLogger which is used" - Should be handles. LogManager class: "also allows to retrieve Logger by there name" - Should be their. LogManager class: "The static LogManager handles the creation, and the release" - The comma shouldn't be there. defaultLogger: "that means it can be assigend" - assigned defaultLogger: "The Logger is returned as a reference that means it can be assigend, thus changing the defaultLogger." - Wording is a bit odd right now, perhaps something like "The Logger is returned as a reference, meaning it can be assigned to change the defaultLogger.". |
July 11, 2014 Re: Review: std.logger | ||||
---|---|---|---|---|
| ||||
Posted in reply to Dicebot | On Friday, 11 July 2014 at 14:38:19 UTC, Dicebot wrote:
> As usual, this review round will last for 2 weeks unless someone asks for delay.
>
> Please share link to this thread via twitter, reddit, G+ and whatever else may be used out there.
On lines 606 and 729 in the comments:
/** This class is the base of every logger. In order to create a new kind of
logger a derivating class needs to implementation the method $(D writeLogMsg).
*/
I think "deriving" would be a better word to use.
|
July 11, 2014 API: string formatting / memory management | ||||
---|---|---|---|---|
| ||||
Posted in reply to Dicebot | Am Fri, 11 Jul 2014 14:36:30 +0000 schrieb "Dicebot" <public@dicebot.lv>: > 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 https://github.com/D-Programming-Language/phobos/pull/1500/files#diff-165d04f1fd1a1db2c37af18580f41fb4R421 The 'frontend' log methods call format and the pass the formatted string to the Logger implementations. I still wonder if we can do better, and if we need to modify the API for that. I've got three ideas: * Using some sort of static buffer in the log* functions. I don't like this that much cause it will limit the msg string length * Use malloc: This way no GC allocations, but still dynamic memory allocation which might not be necessary if you log e.g. to the console or a file. These two options have the benefit that the API can remain unchanged and they could be implemented after the review. The third option requires changes to the API. We overload ------------------ writeLogMsg(ref LoggerPayload payload); with writeLogHeader(ref LoggerPayload header); ------------------ And add another function: ------------------ void put(/*scope*/ const(char)[] msg); ------------------ 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) |
July 11, 2014 Re: Review: std.logger | ||||
---|---|---|---|---|
| ||||
Posted in reply to Dicebot | On Fri, Jul 11, 2014 at 02:59:43PM +0000, Dicebot via Digitalmars-d wrote: > On Friday, 11 July 2014 at 14:39:09 UTC, David Nadlinger wrote: > >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. > > > >Is this for std.* or std.experimental.*? > > > >David > > Deciding this is subject of this review/voting iteration too - it is mostly matter of API stability, how much of a trust reviewers are ready to put into existing API. > > Personally I believe that for something like logging library stabilization period of one release cycle in std.experimental is desirable because wider usage is very likely to result in breaking change suggestions. I vote for std.experimental. We keep talking about it, but never do anything in that direction. Let's start. If it works out poorly, we can always scrap the idea later. But we'll never know if we never do it. (In contrast, putting it directly in std risks the necessity of breaking changes later, which is a Bad Thing. Putting it in std.experimental now does no harm whatsoever -- the worst that can happen is that it's delayed entering std. The best is that breaking changes will not annoy users. So we have nothing to lose.) T -- "Computer Science is no more about computers than astronomy is about telescopes." -- E.W. Dijkstra |
July 11, 2014 Re: Review: std.logger | ||||
---|---|---|---|---|
| ||||
Attachments:
| Haven't had a chance to look closely at the new version yet, but looks pretty good. Couple initial comments:
* Definite vote for std.experimental. Should get a bunch of folks to bang on it before the API has to be locked down. Having it as a dub package first has made it much easier to pick up and use, feel like this is a good path for all std packages to take - get it out there and iterate with user input before pulling in to std.
* The 'Tracer' doesn't feel like it belongs here, and if I understand things correctly won't actually work properly anyway, so should probably be removed.
* What others have said about string formatting. I poked around with this a bit before, but wasn't able to get a clean solution, should probably try again before complaining...
On Fri, Jul 11, 2014 at 9:27 AM, H. S. Teoh via Digitalmars-d < digitalmars-d@puremagic.com> wrote:
> On Fri, Jul 11, 2014 at 02:59:43PM +0000, Dicebot via Digitalmars-d wrote:
> > On Friday, 11 July 2014 at 14:39:09 UTC, David Nadlinger wrote:
> > >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.
> > >
> > >Is this for std.* or std.experimental.*?
> > >
> > >David
> >
> > Deciding this is subject of this review/voting iteration too - it is mostly matter of API stability, how much of a trust reviewers are ready to put into existing API.
> >
> > Personally I believe that for something like logging library stabilization period of one release cycle in std.experimental is desirable because wider usage is very likely to result in breaking change suggestions.
>
> I vote for std.experimental. We keep talking about it, but never do anything in that direction. Let's start. If it works out poorly, we can always scrap the idea later. But we'll never know if we never do it.
>
> (In contrast, putting it directly in std risks the necessity of breaking changes later, which is a Bad Thing. Putting it in std.experimental now does no harm whatsoever -- the worst that can happen is that it's delayed entering std. The best is that breaking changes will not annoy users. So we have nothing to lose.)
>
>
> T
>
> --
> "Computer Science is no more about computers than astronomy is about telescopes." -- E.W. Dijkstra
>
|
Copyright © 1999-2021 by the D Language Foundation