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/[email protected] > > 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
