search

daroczig / logger

A lightweight, modern and flexible, log4j and futile.logger inspired logging utility for R
https://daroczig.github.io/logger
283 stars 41 forks source link

Simplify `log_level()` #182

Open hadley opened 1 month ago

hadley commented 1 month ago

Previously created a function then immediately called it. Now it just inlines the results of that function, which leads to a bunch of simplification. I think this changes suggests that we could deprecate logger() since I doubt there's much use for it outside the package.

This is technically a breaking change since it alters the return value of log_level() and friends. But the current return value was not documented, and it seems unlikely that folks would be dependent on the specific value. Still needs a NEWS bullet though.

daroczig commented 1 month ago

Sorry, I just got to this PR.

You are absolutely right that the return value is not documented :disappointed: But that was explicitly requested e.g. in #26, so I think we should keep that behavior.

Let me think about this one, but I'm very open to any related thoughts.

hadley commented 1 month ago

IMO the additional complexity needed to support that feature is not worth it, especially due to the difficulty (as mentioned in that thread) of exactly what the object should be if there are multiple loggers on the stack. If you really want to keep it, I'm happy to update my code to do so, but my sense would be that it's not going to affect much code in practice, and the simplification will make it easier to apply performance improvements down the road.