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

move ms to the last place #215

Merged
merged 1 commit into from
Oct 28, 2016

Conversation

gorangajic
Copy link
Contributor

when you call debug with more than one param ms diff goes to the second place

var debug = require('debug')('OK');
debug('awesome', 'debug', 'message');  

before
OK awesome +0ms debug message

after
OK awesome debug message +0ms

@steelbrain
Copy link

👍

2 similar comments
@TrejGun
Copy link

TrejGun commented Dec 14, 2015

👍

@mbroadst
Copy link

👍

@steelbrain
Copy link

I wonder what exactly is stopping us from shipping this.

cc @TooTallNate

@mbroadst
Copy link

@steelbrain it may be worth noting that arguments are potentially leaked here, see: https://github.com/petkaantonov/bluebird/wiki/Optimization-killers#3-managing-arguments

@steelbrain
Copy link

@mbroadst I've seen this used in the node core itself (Although they use Array.from instead of Array.prototype.slice.call(arguments) but the concept remains the same), that being said I don't think the performance hit will be significant (unless you have a benchmark proving otherwise)

@gorangajic
Copy link
Contributor Author

not sure it's worth it but I have updated pull request, so now arguments are not leaked

didn't know about Array.from 👍

@mbroadst
Copy link

@steelbrain Array.from I believe will make a new copy of the array, and therefore not leak. I'm not really being a stickler here I was just speculating as to why it hasn't been merged yet :) not to mention the performance hit would be negligible considering debug should be a no-op if you haven't enabled debugging for the given category

@gorangajic Also I'm pretty sure we can't use Array.from here since its ES6 and this needs to target older versions of node?

@mbroadst
Copy link

@gorangajic oops, totally wrote that without looking at your code update please ignore the noise!

var useColors = this.useColors;
var name = this.namespace;
for ( ; i < len; i++) {

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Even though I don't maintain the project, I would still like to hear why i is declared above and not here :)

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

personal preference I'm sure, hoisting and all that

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

can you inline the i in the for loop please?

@mbroadst
Copy link

mbroadst commented Mar 1, 2016

@TooTallNate any updates here?

@world
Copy link

world commented May 30, 2016

this is a great bug fix. I have forked from @TooTallNate who may have abandoned debug from what I can tell. Thank you @gorangajic

@mbroadst
Copy link

cc @TooTallNate ?

@thebigredgeek
Copy link
Contributor

I am reviewing. Once change is applied I'll gladly merge this

@gorangajic
Copy link
Contributor Author

@thebigredgeek updated pull request

@thebigredgeek thebigredgeek merged commit 820f351 into debug-js:master Oct 28, 2016
@thebigredgeek
Copy link
Contributor

@gorangajic merging this. Thanks for the contribution

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

Successfully merging this pull request may close these issues.

6 participants