search

pimterry / loglevel

:ledger: Minimal lightweight logging for JavaScript, adding reliable log level methods to wrap any available console.log methods
MIT License
2.62k stars 157 forks source link

Refactor setLogLevel() #54

Closed cederberg closed 10 years ago

cederberg commented 10 years ago

By moving the no-console-available helper function into realMethod(), most of the code in setLogLevel() becomes obsolete and replaceLoggingMethods() no longer requires any arguments.

pimterry commented 10 years ago

Thanks! I've merged this, because it's definitely an substantial improvement.

I have made some further changes on top that might be of interest though, particularly removing the currentLevel variable (passing that around through state and sideeffects seems messy; nicer to make it an explicit parameter and pass it through), and pulling the 'check for a console and rebind everything' chunk out of realMethod and into its own function for clarity. Take a look at https://github.com/pimterry/loglevel/commit/63d403d10ae9aae27ac3c507054bf8ed36eaf41e if you're interested.

Thanks for contributing!

cederberg commented 10 years ago

Yeah, I noted the changes. The currentLevel variable was added in order to later build a level() function (to perhaps replace setLevel()):

level('info') --> 'info'
level() --> 'info'

I've written some experimental code demonstrating the API I'm aiming for: https://github.com/cederberg/loglevel/blob/experimental/lib/loglevel.js

pimterry commented 10 years ago

I see, ok. I'm not sure I'm totally clear on your reasoning, but I'm certainly interested.

I'm particularly curious about making level() return the current level; what's the use case for that?

And, just out of interest, what's your goal here generally? Why would this API be better for you?

cederberg commented 10 years ago

The main reason to inspect the current log level is to ensure that log output isn't being filtered during debugging. Of course, I shouldn't need to do that. But then again - I've found lots of bugs just by double-checking trivial stuff like that.

Adding getLevel() to the API seems a bit dirty to me. I much prefer the jQuery-style, where a single function handles different number of arguments. Ergo: level().

The second thing I've experimented with is less of a concern for me, but it was requested at work:

var logger = log('blahBlahHandler:')
logger.debug('entering x') --> logs 'blahBlahHandler: entering x'
logger.debug('found value', y); --> logs 'blahBlahHandler: found value ...'

I.e. creating sub-loggers with a prefix to their output. Same log level is reused for all loggers and all are bound directly to console.* functions.