On Saturday, 12 July 2014 at 16:13:22 UTC, Sönke Ludwig wrote:
Overall looks good to me. Some points that haven't been
mentioned so far in this review round:
- Using a class with static members doesn't seem to be very
idiomatic. It seems like the three member properties can simply
be made global and everything should be fine - the
"LogManager." prefix doesn't really add information. This has
been mentioned in the last review round and it's not a very
important point in this particular instance, but we should
really make a decision here that will also decide how future
modules go about this.
- Personally, I really find additional verbose log levels
useful. Currently there is only "trace". Previous proposals had
a generic "verbose(N)" set of levels, but I think it's
important for the proper interaction of multiple libraries to
define a semantic set of verbose levels. A proposal, which has
worked very well me (from low to high):
trace:
as now, used for low level tracing of program flow
debugv:
verbose debug messages, may flood the log
debug:
normal, low frequency debug messages, useful for the
developer
diagnostic:
diagnostic output also potentially useful for the user
this is what you typically would get with the -v command
line switch
the LogLevel enum has quite a lot of free number in between trace
and info and so forth. In combination with a MultiLogger it is
very easy to build verbose logging special to your needs.
- There is a "formatString" string mixin that includes a lot
of if-else cases that seem to do exactly what formattedWrite
would do anyway, is there something that it actually does in
addition to that? Also, why is it a string mixin in contrast to
a simple (template) function? It may be a matter of taste (but
also of compile time/memory), but I'd almost always prefer a
non-string-mixin solution.
- Even if it may be more typing (or maybe not?), actually
writing out the different signatures for each log level and the
c/f suffixes would be very advantageous for the documentation
and for code completion. It would also make the EBNF
unnecessary, which I agree with Johannes looks a little scary.
All of the functions could be based on the generic logl/loglf
functions, so that there wouldn't be much redundancy.
I'm about to change the codegen.
- I'm still really not sure if the "c" versions of the log
functions pull their own weight. They seem to be in line with
trivial convenience functions, which are generally discouraged
in Phobos (with the same issues, such as additional cognitive
load and bigger API size).
Anyone else?
- The functions error(), info(), fatal(), etc. don't follow
the usual rule for functions to start with a verb. The question
is if saving three characters over logError() is worth making
the code more ambiguous for the outside reader (e.g. "does
error() throw an exception? or set some internal error state?"
"does fatal() terminate the process?")
if I change it back, people will argue that that is redundant and
unintuitive. Than I will change it back again and the discussion
starts again.
