Run-time configuration of output

[volks73]

Your simple, verbosity-based logger matched my needs for a project I was working on the other day. Thank you for creating, documenting, and sharing the crate. While working on my project, I noticed that the log statements did not include the all-uppercase level. The level was indicated by the color, which was very helpful and cool, but I asked myself the question: "What happens if colorization is not possible, then how can I tell the log statement level?". I went in search of adding the level to the log statements. This lead to another question of "Can I disable/enable color output?", which led to the final question: "Can I change the various components of the log statement at run-time like I can with the level based on a verbosity?". From these questions comes this pull request that adds run-time configuration of the output for the logger.

This pull request adds a Builder pattern API to change formatting and disable/enable various options. The three core helper functions to create the original output remain for backwards compatibility and maintain simplicity. In creating the Builder API, I added more tests, examples, and documentation. I know this appears like a significant change to the crate and this pull request is coming out of nowhere, but I hope these changes can be included and still maintain the crate's and its API's simple and efficient "feel".

[clux]

Wow. Thanks a lot for this. I do think this is quite a significant change to the module, but it's generally for the better and looks very well thought through. The restructuring is also done very elegantly and there's great example and docs additions as well :+1:

It is a bit breaking with introducing the log level by default though. I have a large CLI crate designed around levels not being there, and having users getting used to certain visual expectations rather than having to read log levels everywhere (actually works well). If the message levels were only displayed if colorization fails/were not set, or a flag to configure these could be set set then this would probably be better (less breaking).

[volks73]

It is a bit breaking with introducing the log level by default though.

Yes, it was only after I submitted the pull request did I realize I changed the default format and did not add any method to disable/enable the level in the tag portion of the log statement, which was kind of the original reasoning for making the changes (ha!).

It should not take too much effort to revert to the previous level-free format as the default and add a method to the Builder API to enable/disable the level in the tag. I like the idea of automatically adding the level if colorization is turned off, but you may want to have the levels with color as well.

It is just a matter of naming the method as level is already used to set the log level directly instead of indirectly through the verbosity. Maybe change the level method to max_level and change the level method to level(bool), where true enables the level printed with the tag portion? The default would be false to be less breaking.

Oh, I guess I would have to revert wrapping the module path in brackets then, too, or only wrap the module path in square brackets if the level is enabled?

How would you like to proceed? Should I make the changes and submit a new pull request?

[clux]

It should not take too much effort to revert to the previous level-free format as the default and add a method to the Builder API to enable/disable the level in the tag. I like the idea of automatically adding the level if colorization is turned off, but you may want to have the levels with color as well.

Yeah, fully agree. A separate flag is useful to override the default is nicest here.

Oh, I guess I would have to revert wrapping the module path in brackets then, too, or only wrap the module path in square brackets if the level is enabled?

Yes please. The brackets make sense if all the information is present, but less so when not.

It is just a matter of naming the method as level is already used to set the log level directly instead of indirectly through the verbosity. Maybe change the level method to max_level and change the level method to level(bool), where true enables the level printed with the tag portion? The default would be false to be less breaking.

Yes, that seems sensible.

How would you like to proceed? Should I make the changes and submit a new pull request?

Any changes you add to master will show up to update this PR. Since you are already in the zone I am happy to just wait for your signal before merging :-)

[volks73]

Okay, I have made the changes such that this pull request does not break backwards compatibility with the default format. The max_level method takes the place of the "old" level method while the "new" level method enables/disables the inclusion of the log level in the tag portion of the log statement.

I did some refactoring relatively to the original pull request after making the changes to (hopefully) make the code easier to read and follow. The config example is updated to use the new level method. I found a small problem where if the level, line numbers, and module path were all disabled, there would be a floating : in each log statement. So, I added a check and if all of these are disabled, then the separator is also disabled. Disabling of the separator occurs by changing it to the empty string, so the init method signature had to change slightly.

The square brackets are only added if the level is included in the tag. I have updated the unit tests to match these changes, along with the documentation. I updated the README slightly to highlight the run-time modification of logging a little bit better.

I believe this is ready for a merge.

[clux]

That's looking great. Cleared up all the breakages and added a bunch more configurations. I'm amused that you've basically rewritten the entire logger in this PR, but thank you so much. This is a great upgrade.

[clux]

Tested a bit more by changing initialization method. One slight edge case that I believe is a bit of a change to the default behaviour:

    loggerv::Logger::new()
        .verbosity(args.occurrences_of("v"))
        .level(args.is_present("level"))
        .line_numbers(args.is_present("debug"))
        .module_path(args.is_present("debug"))
        .colors(!args.is_present("no-color"))

By having module_path which by default is on, being tied to the presence of an optional debug flag, you've effectively made module_path default off; but only if you initialize the logger in this way!

At least if we document it like this then you have this potential confusion. Maybe it's best to invert the module_path setter to no_module_path. To be inline with the defaults. But then the negated part is also a bit awkward to expose semantically. A flag to make the logs have less prefixes? It's not something I would expose, but then again, I would always like to have these.

I'm almost tempted to just not expose changing the module_path functionality at all, and always include the module paths. Got any thoughts on this?

[volks73]

Sorry, long post coming. Yes, I see the confusion and inconsistency. Nice catch! I also now notice a similar problem with the colors method as well. Colors are on/enabled by default, too.

I'm almost tempted to just not expose changing the module_path functionality at all, and always include the module paths. Got any thoughts on this?

I would like the ability to include or not include the module path in the log statement. Logging is useful in two modes: (1) development and (2) support. In development, having the module path is very helpful, if not a necessity. It provides a call chain to pin-point within the source code the exact location of the problem...super handy. In support, it may expose implementation details that would just confuse an end-user of your CLI application, more akin to displaying a panic message instead of a friendly error message. In the support case, the end-user just needs to know that an error or warning occurred and relay that information to the developer. The developer can then enable the module path to narrow down the cause, or instruct the end-user to run the CLI application in a mode that displays the module path. This latter mode is what I was trying to demonstration with the example and the -d,--debug flag.

Exposing the module_path functionality in the API would also enable toggling based on a Debug or Release configuration using conditional compiler (cfg) attributes in the manifest or source code that could also address these two modes. Now, within the context of a server-side application or internal tool, these two modes do not necessarily exist and the module path is always useful; hence, the default is to include it.

Maybe it's best to invert the module_path setter to no_module_path.

I like this idea, but instead of replacing the method I would add the "no" method. I agree the negated part would be a bit awkward, but maybe we should give the crate user (CLI application developer) the option to use either depending on his or her needs and/or tastes? Given that the colors method has a similar problem, I would suggest adding the no_module_path() and no_colors() methods with no parameters and leave the module_path and colors methods as is. A developer can use the no_module_path and/or no_colors methods to change the defaults at compile-time while avoiding weird boolean semantics, but the API becomes more "self-documenting" and consistent about defaults. If run-time configuration is desired, similar to the current config example, then the current module_path and colors methods can be used.

I would also propose changing the example to the following to avoid confusion within the examples, which is always a good idea:

#[macro_use] extern crate log;
extern crate loggerv;
extern crate clap;

use clap::{Arg, App};

fn main() {
    let args = App::new("app")
       .arg(Arg::with_name("v")
            .short("v")
            .multiple(true)
            .help("Sets the level of verbosity"))
       .arg(Arg::with_name("debug")
            .short("d")
            .long("debug")
            .help("Adds the line numbers to log statements"))
       .arg(Arg::with_name("no-color")
            .long("no-color")
            .help("Disables colorized output"))
       .arg(Arg::with_name("no-module-path")
            .long("no-module-path")
            .help("Hides the module path from the log statement.")
       .arg(Arg::with_name("level")
            .short("l")
            .long("level")
            .help("Adds the log level to the log statements. This will also surround the module path in square brackets."))
       .get_matches();

    loggerv::Logger::new()
        .verbosity(args.occurrences_of("v"))
        .level(args.is_present("level"))
        .line_numbers(args.is_present("debug"))
        .module_path(!args._is_present("no-module-path")
        .colors(!args.is_present("no-color"))
        .init()
        .unwrap();

    error!("This is always printed to stderr");
    warn!("This too is always printed to stderr");
    info!("This is optional info and printed to stdout");
    debug!("This is optional debug and printed to stdout");
    trace!("This is optional trace and printed to stdout");
}

I separated out the toggling of the module path from the -d,--debug flag and gave it is own --no-module-path flag. I also removed the short name for the --no-colors flag to be consistent with the new --no-module-path flag, which also does not have a short name. I think only having the long name for these options makes it a little more clear that the defaults include colors and the module path and turning them off is more of an edge case. Having to use the no_module_path and no_colors methods in this example would be awkward.

I can add in these methods and provide a "compile-time" example with the new "no" methods if you like? I would rename the config example to run-time-config and create a compile-time-config example.

Here is a draft of the compile-time-config example:

#[macro_use] extern crate log;
extern crate loggerv;

use log::LogLevel;

fn main() {
    loggerv::Logger::new()
        .max_level(LogLevel::Info)
        .level(true)
        .no_module_path()
        .no_colors()
        .init()
        .unwrap();

    error!("This is always printed to stderr");
    warn!("This too is always printed to stderr");
    info!("This is optional info and printed to stdout");
    debug!("This is optional debug and printed to stdout");
    trace!("This is optional trace and printed to stdout");
}

I added usage of the max_level and level methods to demonstrate setting the log level without verbosity and include the level because colors are turned off. All of which are deviations from the default, but still allow for the configuration of the output that was the original intention of this pull request.

[clux]

Exposing the module_path functionality in the API would also enable toggling based on a Debug or Release configuration using conditional compiler (cfg) attributes in the manifest or source code that could also address these two modes. Now, within the context of a server-side application or internal tool, these two modes do not necessarily exist and the module path is always useful; hence, the default is to include it.

I kind of like the idea of having both methods available now that you've explained your use case, but if it's for conditional compilation, maybe the example could make a #[cfg] based choice rather than ever presenting the more awkward --no-X flags?

Something like:

    if cfg!(build = "release") {
        loggerv::Logger::new()
            .no_module_path()
            .no_colors()
    } else {
        loggerv::Logger::new()
    }.max_level(LogLevel::Info)
     .init()
     .unwrap();

This would highlight which parts are changed by conditional compilation and which aren't.

[volks73]

I added a cfg-config in addition to the compile-time-config example using your suggestion. However, the build = "release" attribute does not exist. I could not get the example to work with it or a cfg!(debug), but if I changed it to the following:

if cfg!(debug_assertions) {
    loggerv::Logger::new()
        .max_level(LogLevel::Trace)
} else {
    loggerv::Logger::new()
        .no_module_path()
        .level(true)
        .max_level(LogLevel::Info)
}.init().unwrap()

based on this discussion, the conditional compilation example would work.

Next, I also tried the examples on Windows 10. The coloring did not work on any of them because ANSI compatibility is disabled by default for the Windows 10 cmd.exe. Luckily, the ansi_term crate has a function for enabling ANSI support in Windows 10 (see the README and note under the Basic Usage section), but it is only available in v0.10 not v0.9. So, I bumped the dependency in the manifest and added the function to each of the examples with some documentation to help others enable coloring on Windows. Now coloring works in Windows 10 too!

I also decided to change the clap example to quick. The example is a quick demonstration of this crate, not necessarily of the clap crate, so I thought a name change would be appropriate. The clap crate also uses a "quick" naming scheme for some of its examples. I have updated the README and manifest to reflect this name change and included information about running all of the examples.

I added the no_module_path and no_colors methods to the Builder pattern API as well.

[volks73]

I'm amused that you've basically rewritten the entire logger in this PR...

It was not my intention to do a re-write or take over in any way, but I kind of got in a groove, had some free time (which almost never happens), and it just poured out. Originally, I just wanted to add some functionality and contribute a little.

In the mean time, I'll add you as a collaborator here if you wish to add or tweak things in the future.

Super cool, thanks. I will keep an eye out for the release so I can integrate in my current and future projects.

[clux]

Hah, it's not a problem when you make it this much better. Very much appreciate the thought you put into this simple crate.

I've added you to Cargo.toml, published gh-pages and and released 0.5.1 (unfortunately made a mess of 0.5.0). Hope this is acceptable :smile: