Re: Proper loglevels: cmd-line option --loglevel=NONE/ERROR/WARN/PROGRESS/INFO/DEBUG (issue4822055)

[reinhold . kainhofer]

http://codereview.appspot.com/4822055/diff/1/lily/all-font-metrics.cc
File lily/all-font-metrics.cc (right):

http://codereview.appspot.com/4822055/diff/1/lily/all-font-metrics.cc#newcode91
lily/all-font-metrics.cc:91: debug_output ("[" + string (pango_fn),
true); // start on a new line
On 2011/07/30 17:10:00, Ian Hulin (gmail) wrote:
> Why do the logging output in two calls?  Why not
>   debug_output ("[" + string (pango_fn)+ "]", true); // start on a new
line
>         here and lose line 102?

Because the following four lines until the "]" might also print out some
debug output. We want to keep everything printed out by this one font
inside the brackets, so it's clear that the debug output is really from
this font.

http://codereview.appspot.com/4822055/diff/1/lily/font-config.cc
File lily/font-config.cc (right):

http://codereview.appspot.com/4822055/diff/1/lily/font-config.cc#newcode56
lily/font-config.cc:56: debug_output (_f ("adding font directory: %s",
dir.c_str ()));
On 2011/07/30 17:10:00, Ian Hulin (gmail) wrote:
> Capitalize "Adding" - for consistency with other messages, which start
> "Initializing..." and "Building..."

Thanks for spotting this. Previously this was a warning, which should
start with a lowercase letter. Now it's only debug output, so we need a
capital.

http://codereview.appspot.com/4822055/diff/1/lily/input.cc
File lily/input.cc (right):

http://codereview.appspot.com/4822055/diff/1/lily/input.cc#newcode87
lily/input.cc:87: ::print_message (s);
On 2011/07/29 20:46:33, Reinhold wrote:
> Can leave out the :: here

No, I can't :(

http://codereview.appspot.com/4822055/diff/1/lily/input.cc#newcode94
lily/input.cc:94: print_message (_f ("error: %s", s));
On 2011/07/30 17:10:00, Ian Hulin (gmail) wrote:
> print_message ( _f ("fatal error: %s", s));

> Why do you pass s here and in the similar place in
> warn.cc you need to pass s.c_str()?

No idea, I simply copied the existing code without changes...
I can probably remove the .c_str() in warn.cc...

http://codereview.appspot.com/4822055/diff/1/lily/program-option-scheme.cc
File lily/program-option-scheme.cc (right):

http://codereview.appspot.com/4822055/diff/1/lily/program-option-scheme.cc#newcode259
lily/program-option-scheme.cc:259: "Was berbose output requested, i.e.
loglevel at least @code{DEBUG}?")
On 2011/07/30 17:10:00, Ian Hulin (gmail) wrote:
> On 2011/07/29 19:51:00, Reinhold wrote:
> > should be "verbose"

> Or maybe "Was full debug output requested, i.e. loglevel at least
@code(DEBUG)?"

That's ambiguous, as "fulldebug" is also a debuglevel stronger than just
DEBUG.

http://codereview.appspot.com/4822055/diff/1/lily/program-option-scheme.cc#newcode271
lily/program-option-scheme.cc:271:
On 2011/07/30 17:10:00, Ian Hulin (gmail) wrote:
> Nice feature.
> If you're allowing this to be accessed from Scheme, how about
translating the
> number back to the keyword value as input on the command line (NONE,
PROGRESS,
> ...)

Or probably do it the other way round: Replace it with a function
ly:loglevel?, with one argument, returning whether the requested
loglevel is set. E.g. (if (ly:loglevel? 'PROGRESS) ....)

http://codereview.appspot.com/4822055/diff/1/scm/lily.scm
File scm/lily.scm (right):

http://codereview.appspot.com/4822055/diff/1/scm/lily.scm#newcode288
scm/lily.scm:288: ;;       a newline in this case
On 2011/07/30 17:10:00, Ian Hulin (gmail) wrote:
>      (ly:debug ( _ "\t[Loading ~a] ...\n") filename))))

Again, the reason for [ and ] printed at the beginning and at the very
end of this function is that all log messages printed out during loading
will be bracketed by [ ... ]. This makes it easier to see whether a
message was caused inside the loaded file or sometime after the loading.

> (define-public (ly:load x)
>    (if (not file-name)
>        (ly:error (_ "cannot find: ~A") x))
>    (primitive-load-path file-name))
> >>>>>>
> ly:load is getting a re-write in any case as part of the fix for T1686

http://codereview.appspot.com/4822055/