refactor the crate to separate the provided configurable formater and the optional custom formatter

[djugei]

This crate does three things:

1. It parses environment variables into a verbosity configuration for individual modules (Env)
2. It filters log messages against said configuration (Logger.filter)
3. It provides a configurable formater (Logger.format)

You will generally read en environment, convert it to a filter and then combine the filter and a formater into a Logger which you install as default logger.

Technically all three are usable in isolation.
In practice almost everybody is using 2., most people will be using 1. with slight modifications to the default logging level, for example using verbosity flags in clis.

3 is the most commonly exchanged. This is reflected by the builder giving you the option of providing a custom formater.

This functionality should be moved to the Logger. If you want a custom formater you do not interact with the provided configurable formater in any way, but are still forced to use the type to create a Logger.

The current behaviour increases the internal complexity of the Builder struct and leads to weird api crincles. For example after calling Builder.format(|my| fn) technically every try to modify the configurable formatter (ex. format_level) should panic, as that is programmer error (unless ofc it is followed by a call to default_format).

This issue proposes to create a build_with_custom_formatter function instead. This would allow fmt::Builder to be retired.

Also since 90% of the api on the Builder is formatter options a single fn config_builder(|f: formatter|) could reduce duplication.

[epage]

Note that we offer (1) and (2) as a separate crate, env_filter.

However, it sounds like your concern is more with the API of env_logger itself.

We recently did #360. #361 takes this a step further but we're holding off on that because it is a breaking change. I had considered whether I wanted to force people over into using ConfigurableFormat but I was concerned with the loss of ergonomics.

I'll admit, I'm a bit unclear on some of your suggestions. I think the use of pronouns (e.g. this) and hand-waiving about what the API would be (e.g. "moving to the logger") are making the idea unclear.

I also feel like the problem being addressed is not very clear. I don't see how something in this direction would reduce complexity. I also don't see "api circles". There is some modality to the API that could be clarified.

[djugei]

thank you for the feedback. Yes this is about changing the api of env_logger.

I agree that my text is a little unclear in points, i have partially implemented my idea in #363, maybe that clears up things a bit. it moves the functionality to Logger, maybe it should go to logger::Builder instead.

Moving people to ConfigurableFormat is a step in the right direction imo.

i meant crinkles, like creases/folds, unsmooth parts. trying to configure the ConfigurableFormat while already having set a custom FormatFn is a programmer error. Optimally this should be a compiler error/not possible to be in line with the making illegal states unrepresentable idea.

This issue was motivated by altering env_logger for a pr and being confused by the internal structure of the fmt::Builder.
So that is what this issue mainly addresses. Though i do remember as a first time user to find the different builders confusing.

to be slightly more precise about what the api should look like:
i mainly want to decouple things that don't have any relation, this means not having the user-provided formatting function interact with the crate-provided ConfigurableFormat.
It would also mean having the configuration for the ConfigurableFormat on ConfigurableFormat and not on logger::Builder.

In fact i am reasonably sure that this crate could do without having any builder at all, simply initializing to a default value and providing configuration, but im not sure that is the best possible api.

Hope that clears things up.

[epage]

i meant crinkles, like creases/folds, unsmooth parts. trying to configure the ConfigurableFormat while already having set a custom FormatFn is a programmer error. Optimally this should be a compiler error/not possible to be in line with the making illegal states unrepresentable idea.

There are competing needs from an API

• Ease of use
• Clarity
• Static vs dynamic

Requiring users to look over 3 distinct builders, create them, and then pass them into a Logger or a Builder would not be easy to use.

We could have Builder be generic over custom vs configurable formatter. This would avoid most of the problems with the above but it moves us from being dynamic to static. Its easy to be static and then have people come to you with dynamic use cases to make the switch as its adding features. However, going the other way is removing features and its hard to evaluate how important that is for users. I also prefer to avoid generics in APIs unless there is a strong enough case because they can make things harder for the newest of users.

We do have the current clarity issue with custom formatters but that seems like its more for a minority case and I suspect its not too bad.

In fact i am reasonably sure that this crate could do without having any builder at all, simply initializing to a default value and providing configuration, but im not sure that is the best possible api.

Logger is made of the following parts

• filter
• writer
• format

Each requires what could be an expensive build operation. I would prefer to do those lazily through a top-level builder, rather than making assumptions of the users intent and then having them overwrite that.

[djugei]

We could have Builder be generic over custom vs configurable formatter. This would avoid most of the problems with the above but it moves us from being dynamic to static. Its easy to be static and then have people come to you with dynamic use cases to make the switch as its adding features. However, going the other way is removing features and its hard to evaluate how important that is for users. I also prefer to avoid generics in APIs unless there is a strong enough case because they can make things harder for the newest of users.

That is an interesting Idea, i am pretty sure having a Builder and then implementing functions for the more specific Builder is an elegant solution. Could even alias Builder to DefaultBuilder or smth.

I have updated #363 to in light of this discussion. By optionally passing the custom formatfn in the build method its both explicit and skips generics.

There is two more clarity/simplification changes i would like to do:

1. the Builder methods currently take &mut Self, requiring storing the built boolean, is there a good reason for not just taking self? that would express the same semantics but in the type system. I'm sure there is a reason but i can't find it rn.

2. (as mentioned earlier) instead of duplicating the api surface of ConfigurableFormat in the Builder, provide on single

 fn config_format(self, FnOnce(ConfigurableFormat) -> ConfigurableFormat) -> Self

(or assuming 1 does not happen)

 fn config_format(&mut Self, FnOnce(&mut ConfigurableFormat)) -> &mut Self

Each requires what could be an expensive build operation. I would prefer to do those lazily through a top-level builder, rather than making assumptions of the users intent and then having them overwrite that.

I am unsure if the one-time cost of initialization is that high (or that it even gets compiled in at all if overwritten) but in general agreed.