Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

add set_output_format_custom for Config. #123

Open
wants to merge 4 commits into
base: master
Choose a base branch
from

Conversation

coucya
Copy link

@coucya coucya commented Mar 23, 2023

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
Copy link
Owner

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
Copy link
Author

coucya commented Mar 24, 2023

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
Copy link
Author

coucya commented Mar 24, 2023

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
Copy link
Owner

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:

  • Would it be possible to make a few of the FormatBuilder function order-independent? There is no reason I should not be able to call level_color later. 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.
  • Maybe we could move everything except the actual order of elements out of the Format. That way we could keep the ConfigBuilder functions to set level-filters or colors. This means however adding custom literals (that maybe should be filtered as well) gets a little more complicated. So your example would become something like the following, so all literals between begin* and end calls would also be filtered:
FormatBuilder::new()
            .begin_time()
            .time()
            .wrap_space(true)
            .end()
            .begin_level()
            .literal("[")
            .level()
            .literal("]")
            .end()
            .begin_thread()
            .thread()
            .wrap_space(true)
            .end()
            .begin_target()
            .target()
            .end()
            .literal(": ")
            .begin_location()
            .literal("[")
            .location()
            .literal("]")
            .end()
            .begin_args()
            .wrap_space(true)
            .end()
            .build()

Are these changes acceptable?

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

@coucya
Copy link
Author

coucya commented Mar 24, 2023

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.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

2 participants