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

Open up the architecture #83

Open
piegamesde opened this issue Jul 8, 2021 · 4 comments
Open

Open up the architecture #83

piegamesde opened this issue Jul 8, 2021 · 4 comments

Comments

@piegamesde
Copy link

I'm trying to switch to using this crate from a more simple logger, and I ended up reading a lot of the internal code. I've noticed that every feature is part of a giant enum. This greatly restricts the ability to locally add new outputs or to customize existing ones, for example.

My suggestion is to factor out an interface into an Output trait.

@daboross
Copy link
Owner

daboross commented Jul 8, 2021

Does using the log::Log trait work for your use case?

You can chain fern loggers with regular log loggers by boxing them. The fern documentation lacks examples for this, but it should work with any log::Log logger with

dispatch.chain(Box::new(logger) as Box<dyn log::Log>)

If this works for your use case, I'd rather fix this by adding a new method to Output to give more visibility to this than overhauling the internal structure.

fern uses an enum rather than a trait to avoid overhead from boxing and dynamic dispatch. Admittedly, this is a very small amount of overhead, and if I were redesigning it from scratch today I think a trait would be better. Or really, we could just use log::Log and get rid of Output entirely.

But I don't think that overhaul should be necessary to allow extending things. The Output enum does have a catch-all Box<dyn log::Log> variant, and that should work for most if not all custom log needs.

@daboross
Copy link
Owner

daboross commented Jul 8, 2021

Not to say that I'd be against redoing this with an entirely trait-based approach, either, though!

I think if we do turn Output into a trait, it should probably be something pretty simple, like

trait Output: log::Log {
    fn max_used_level(&self) -> log::LevelFilter;
}

I'd imagine all the methods currently on the Output struct could become simply functions present in fern - like, fern::Output::file becomes fern::file.

We could then remove a lot of code dealing with transforming things from fern::builders::Output into fern::log_impl::Output, and generally simplify things.

I'm not likely to work on a refresh like this myself any time soon, but I'd definitely be open to changes implementing it - or something similar, if you've got different ideas for how we should restructure the crate.

I just wanted to highlight in my earlier comment that I don't think the redesign is necessary in order to have extensibility - and in fact, we have a degree of extensibility right now.

@piegamesde
Copy link
Author

Or really, we could just use log::Log and get rid of Output entirely.

This. It mostly boils down to the question "how much more does Output do than simply multiplex Logger implementations?"

I just wanted to highlight in my earlier comment that I don't think the redesign is necessary in order to have extensibility - and in fact, we have a degree of extensibility right now.

Gotcha. I can easily hack about everything on to every logging crate, but it quickly gets me to the point where a manual Logger implementation specialized for my use case would have been easier. Which kind of defeats the point.

I'm currently thinking about throwing some time at this. What's this crate's stance on API-breaking changes, given that it hasn't hit 1.0 yet?

@daboross
Copy link
Owner

daboross commented Jul 9, 2021

I'm currently thinking about throwing some time at this. What's this crate's stance on API-breaking changes, given that it hasn't hit 1.0 yet?

We aren't at 1.0, but about 70% of the reason for that is that log isn't at 1.0 yet.

I definitely prefer keeping them to a minimum, but they're not out of the question. I guess - if it adds significant value, I'm willing to make a release, but outside of that I'd rather keep all updates backwards-compatible, and keep breaking changes in a separate branch to be released next time we need a new major version because of a new major log version.

If there are features this unblocks, or if it makes the API significantly nicer to use, I'd be willing to make a new major release for it. Otherwise, I'd be glad to have the changes on hand next time there's a reason for a major release, but I wouldn't make a release just for that.

Right now I'm leaning towards this not being worth a major release? But that's mostly because in my eyes the biggest benefit is code clarity inside the codebase, and that's not something exposed very much to the average user.

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

No branches or pull requests

2 participants