Drakulix / simplelog.rs

Simple Logging Facility for Rust
https://docs.rs/simplelog/
Apache License 2.0
433 stars 73 forks source link

add set_output_format_custom for Config. #123

Open coucya opened 1 year ago

coucya commented 1 year ago

Create a custom Format through the FormatBuilder,

let format : Format = FormatBuilder::new()
    .time_with_level_and_wrap_space(Level::Trace)
    .literal("[")
    .level()
    .literal("]")
    .thread_with_level_and_wrap_space(Level::Trace)
    .target()
    .literal(": ")
    .literal("[")
    .location()
    .literal("]")
    .args_with_level_and_wrap_space(Level::Trace)
    .build();

let config = ConfigBuilder::new()
    .set_output_format_custom(format)
    .build();

This will create a format similar to the original, starting with the time, followed by a level wrapped in square brackets, and so on. The xxx_with_level_and_wrap_space series API maintains a space between the modified fragment and other fragments, while there is only one space between two xxx_with_level_and_wrap_space. The parameter represents the level that can be output, similar to the original set_time_level. Consider removing set_time_level and so on ?. Currently, these APIs are not designed very well, but I want to discuss the feasibility of this solution.

Drakulix commented 1 year ago

Oh wow, that is quite a significant change.

I have no done a thoroughly review on the actual code, but I think this needs to go back to the drawing board for a bit, because the builder api is kinda... meh. Like you noted yourself.

I think in general such a solution can work and can be accepted as a PR.

But I would strongly prefer having some kind of macro to build a custom formatter. Something like the following would make this a usable api in my opinion, that serves the "simple" aspect of the crate and would even allow us to easily add more optional blocks to the log message later:

formatter!("<TRACE>{time:rfc2822}</TRACE> [{level:<5}] <TRACE>{thread}</TRACE> {target:<target_padding}: <DEBUG>[{location}]</DEBUG> {args}", target_padding = target_padding)

Do you think that is something you would be willing to build?

coucya commented 1 year ago

To be honest, I prefer the builder to macros, although it seems more verbose.

However, I believe that macros can be implemented on the basis of the builder.

I redesigned the builder's API to make it clearer (I think so).

In my design, the Format consists of FormatPart,

FormatPart have types such as Time, Level, Location ... , Each FormatPart has its own attributes, such as level_color, level_filter, padding, and so on.

When outputting, just output them in order and their respective attributes.

My commits may have a significant impact on the internal project, and multiple fields in Config may be affected.

There are functional duplications between the new method and the original method. For example:

let format : Format = FormatBuilder::new()
    .begin_level()
    .level_color(Level::Error, Color::Red)
    .end()
    .build();
// and
config_builder.set_level_color(Level::Error, Color::Red);

Both of them can set colors for level_color according to different levels. The new method can not only set colors for level_color, but also set colors for other FormatPart.

In commits just now, I changed the old method so that it cannot take effect and can only use the new method.

The compatible way I came up with is to traverse each level FormatPart in Format and modify its level_color, not yet implemented

Are these changes acceptable?

coucya commented 1 year ago

The same problem as before, the function is duplicated,

let format : Format = FormatBuilder::new()
    .begin_time()
    .filter_level(LevelFilter::Debug)
    .end()
    .build();
// and
config_builder.set_time_level(Level::Error, Color::Red);

The compatible method I came up with is the same as before, but it has not been implemented either.

Drakulix commented 1 year ago

However, I believe that macros can be implemented on the basis of the builder.

That would be a good solution.

I redesigned the builder's API to make it clearer (I think so).

Sounds better, I will take a look at the actual code later, but...

There are functional duplications between the new method and the original method. For example:

let format : Format = FormatBuilder::new()
    .begin_level()
    .level_color(Level::Error, Color::Red)
    .end()
    .build();
// and
config_builder.set_level_color(Level::Error, Color::Red);

Both of them can set colors for level_color according to different levels. The new method can not only set colors for level_color, but also set colors for other FormatPart.

In commits just now, I changed the old method so that it cannot take effect and can only use the new method.

The compatible way I came up with is to traverse each level FormatPart in Format and modify its level_color, not yet implemented

I am not sure, I like having the same functionality exposed in multiple ways.

If we merge this custom formatter implementation, I would expect the formatter to become the only api to change the formatting. Which means dropping all the config functions, that can be used now or at least deprecate them, let them traverse the FormatParts and change the color, but warn the user, that this api is going to be dropped in future versions and that they should switch to the new formatter api.

However I don't want to make it super difficult for users to change something simple like the color, that used to be one call before by instead requiring them to build a completely custom format, so here are a few ideas how to solve that:

Are these changes acceptable?

Except for the open question outlined above, I would say yes.

coucya commented 1 year ago

I would expect the formatter to become the only api to change the formatting. Which means dropping all the config functions, that can be used now or at least deprecate them, let them traverse the FormatParts and change the color, but warn the user, that this api is going to be dropped in future versions and that they should switch to the new formatter api.

That's what I think

Would it be possible to make a few of the FormatBuilder function order-independent?

yes, Is the following method OK?

FormatBuilder::new()
    .begin()
    .set_level_color(Level::Error, Color::Red)
    .literal("[")
    .level()
    // .set_level_color(Level::Error, Color::Red)
    // The location is irrelevant, it can modify all FormatPart between begin() and end().
    // filter_level similarly.
    .literal("]")
    .begin()
    // Can be nested, the nested interior is not affected by external influences, 
    // and the default format is used before being modified.
    .end()
    .end()
    // The outermost is an implicit begin() and end().
    // This will modify the outermost FormatPart.
    .set_level_color(Level::Error, Color::Red)
    .build()
FormatBuilder::default().set_level_color(Level::Error, Color::Red).build()

The code is available in the above method, but it not only sets the level_color of level FormatPart, but also sets other level_color such as time FormatPart.

and

However I don't want to make it super difficult for users to change something simple like the color, that used to be one call before by instead requiring them to build a completely custom format,

That way a user, that just wants to use the default format with custom colors could do something like FormatBuilder::default().set_level_color(Level::Error, Color::Red).build(). FormatBuilder::default() would fill the builder with all the default blocks.

So I don't know how to simply set the color. ConfigBuild::set_level_color can be used to modify the colors of all level FormatPart in Config::custom_output_format, but what about the others?

Also, if you use ConfigBuild::set_level_color first and then use ConfigBuild::set_output_format_custom, ConfigBuild::set_level_color will not take effect.