Early review of std.logger

[Robert Schadek]

On 11/04/2013 02:46 PM, Dicebot wrote:
> On Monday, 14 October 2013 at 11:39:52 UTC, Dicebot wrote:
>> As `std.logger` is still marked as "work in progress" this thread is
>> less formal that typical pre-voting review. Goal is to provide as
>> much input about desirable `std.logger` functionality and current
>> state and let module author to use this information for preparing
>> proposal for actual review / voting.
>>
>> Lets unleash the forces of constructive destruction.
>>
>> ===== Meta =====
>>
>> Code: https://github.com/D-Programming-Language/phobos/pull/1500/files
>> Docs: http://burner.github.io/phobos/phobos-prerelease/std_logger.html
>>
>> First announcement / discussion thread :
>> http://forum.dlang.org/post/mailman.313.1377180809.1719.digitalmars-d@puremagic.com
>
> Ok, finally making some conclusions.
>
> ===================
> As a review manager
> ===================
>
> This what needs to be done before moving to next review stage:
>
> 1.
>
> Judging by review comments it is clear that some more efforts need to
> be put into demonstrating how existing platform can be used as a
> standard API for more complex logging functionality. Addition of
> `MultiLogger` is good thing here, but providing either separate
> article on topic or extra module that demonstrates enhancing existing
> system feels like a good way to avoid confusion.
>
> That said, I do agree that std.logger itself should not be "all
> batteries included" module as it does contradict overall Phobos style.
> It may be extended in future once we start using sub-packages much
> more but right now having good standard API is much more important.
I looked at journalctl, it looks promising any small enough to fit in
the docu as an example.
>
> 2.
>
> Standard logger must be made thread-safe by default. Out of the box
> safety is very important for standard library.
>
on my todo list: My first idea is to make the global logger shared and
StdIoLogger and FileLogger synchronized in the right places.
> 3.
>
> Making default `log` a conditional logger does not seem to be gladly
> accepted decision. I'd recommend to swap it with `logf` and rename to
> `logIf`.
I don't see the fuss. the bool in front is just a transparent. But If
people really want to write the 2 extra chars, be my guest.
>
> Naming overall should be reviewed to make sure there is no word
> duplication in typical usage pattern - Phobos does favor plain
> procedural/functional approach as default and D module system makes it
> unnecessary to encode namespaces into names.
>
> ====================
> Personal preferences
> ====================
>
> It is a matter of personal taste but any single thing in that list
> will make me vote "No" on logger proposal:
>
> 1.
>
> API it too verbose.
> I want stuff as short and simple as this:
> ```
> import std.logger;
> warn("1");
> inform("2");
> die("3");
> ```
> I don't want to refer logger class instances explicitly for any
> typical task.
>
> Stuff like `auto logger = new MultiLogger();
> logger.insertLogger(someOtherLogger);` is redundant beyond absurd -
> just how many times one may insert word "logger" in a single sentence? :)
>
no and yes, the warn inform, maybe.
The MultiLogger part ... it needs a better constructor. Put it on my
todo list
> 2.
>
> Logging payload needs to include fully qualified module name. This is
> necessary for enabling module-local output.
Easy fix, put it on my todo list.
>
> 3.
>
> I am not sure if proper global vs local log separation can be done
> without providing two distinct default instances in std.logger (as I
> have already said, I don't consider solutions which imply tracking
> class instance manually).
I'm not sure as well
>
> 4.
>
> Static "namespace" classes are redundant and should be replaced with
> module-scope globals.
not sure either


Thank you for taking effort to work on this "soon to be" review.
Robert