-
Notifications
You must be signed in to change notification settings - Fork 139
refactor the crate to separate the provided configurable formater and the optional custom formatter #365
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
Comments
Note that we offer (1) and (2) as a separate crate, However, it sounds like your concern is more with the API of 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 I'll admit, I'm a bit unclear on some of your suggestions. I think the use of pronouns (e.g. 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. |
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. to be slightly more precise about what the api should look like: 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. |
In thinking over rust-cli#365 and looking over `Logger`, this is an orthogonal, top-level concept Maybe at a later point, we can consider if we should revisit how this is exposed in the API.
There are competing needs from an API
Requiring users to look over 3 distinct builders, create them, and then pass them into a We could have 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.
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. |
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:
fn config_format(self, FnOnce(ConfigurableFormat) -> ConfigurableFormat) -> Self (or assuming 1 does not happen) fn config_format(&mut Self, FnOnce(&mut ConfigurableFormat)) -> &mut Self
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. |
This crate does three things:
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.
The text was updated successfully, but these errors were encountered: