logger level methods like logger.info(...) behave the same as logger.log('info, ...) #246

Merged
merged 1 commit into from May 9, 2013

Conversation

Projects
None yet
7 participants
Contributor

dougnukem commented May 3, 2013

Logger level methods like logger.info now support:

  • passing metadata
logger.info('test message', {foo: 'bar'});
  • interpolating string args
logger.info('test message %d', 123);
  • should simply behave as a shortcut for calling logger.log(level, ...)

It appeared that logger level helper methods like logger.info() weren't behaving the same as if I made the underlying logger.log('info', msg, metadata, callback) style call.

I simplified the common.js setLevels to simply proxy the call to target.log, it was previously attempting to parse out callback and metadata and I first noticed that metadata was not being passed through correctly.

This pull request was initiated based on my question on StackOverflow: http://stackoverflow.com/questions/16349288/winston-js-info-method-not-passing-metadata

logger level methods (like info()) now behave exactly like logger.log…
…('info', ...) (metadata, string interpolation etc)
Contributor

kenperkins commented May 6, 2013

I believe we should put this PR on hold until we can ascertain why the regression was introduced.

https://gist.github.com/kenperkins/5528711

@indexzero: Sorry to stalk :) but I think you might be able to help based on a quick git blame. Any idea why the regression happened to 0.7.1 ?

Contributor

dougnukem commented May 7, 2013

I believe it was introduced by this commit 003808b and having something to do with the ability to do string interpolation using util.format introduced by afe15ba. I tried reverting the common.js and running the logger-test.js introduced by the new "util.format" changes and it passed, so I'm not exactly sure why it was introduced.

I'm too. I was surprised finding out, what helper methods behave different. I've spent about 30 minutes seeking bugs in my code :)

Contributor

kenperkins commented May 8, 2013

FYI, I opened #248 to specifically address the regression.

There are already more issues, than lines of code fixing this little bug :)

mmalecki added a commit that referenced this pull request May 9, 2013

Merge pull request #246 from dougnukem/log-level-metadata
logger level methods like logger.info(...) behave the same as logger.log('info, ...)

@mmalecki mmalecki merged commit 6cf229d into winstonjs:master May 9, 2013

1 check passed

default The Travis build passed
Details
Contributor

mmalecki commented May 9, 2013

Merged, huge thanks, Doug. Sorry it took so long. I'll release after testing it out and checking if it doesn't break our CLI apps.

@dougnukem dougnukem deleted the dougnukem:log-level-metadata branch May 9, 2013

johan commented May 9, 2013

Thanks; I am also eagerly awaiting 0.7.2 to update dependencies to a version with working multi-arg info/warn/error.

Is this going to be released soon? I'm running off master locally, and the patch appears to correct the issue in 0.7.1.

Also, any chance that Broadway will be updated to use 0.7.2? It's currently using 0.6.2, which is a bit annoying.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment