Option to prefix date to messages #3

Merged
merged 1 commit into from Aug 23, 2012

Projects

None yet

2 participants

@brunomorency

Hi Tristan -- Here's a commit that adds an option to prefix date+time to all log messages. It's implemented through a logly.options() method that integrates both this 'date' option as well as the 'color' option added recently. To keep it backwards compatible, the logly.color() calls are still there but are just the same as logly.options({color: true}).

@tristanls tristanls commented on the diff Aug 23, 2012
if(typeof colour === 'undefined') colour = noColour;
+
+ // add date as message prefix
+ var datePrefix = (options.datePrefix) ? (new Date()).toString() + ' ' : '';
@tristanls
tristanls Aug 23, 2012

How do you feel about ISO8601via toISOString() ?

perhaps we can pass in a date format instead of just true? if the format is "ISO" or "ISO8601" it does it that way?

@brunomorency
brunomorency Aug 23, 2012

I like the idea of letting users just put a boolean value to get a default or a string to get their desired format.

@tristanls tristanls commented on the diff Aug 23, 2012
@@ -134,11 +140,15 @@ exports.name = function( applicationName ) {
name[ process.pid ] = applicationName;
};
-exports.colour = function( bColour ) {
- colourText = bColour === true;
+exports.options = function( opts ) {
+ options.colourText = (('color' in opts) ? (opts.color === true) : (('colour' in opts) ? (opts.colour === true) : options.colourText));
+ options.datePrefix = (('date' in opts) ? (opts.date == true) : options.datePrefix);
@tristanls
tristanls Aug 23, 2012

is opts.date on purpose? or is it supposed to be opts.datePrefix? or both? (also, how about ISO8601 format?)

@brunomorency
brunomorency Aug 23, 2012

'date' is how the option is exposed to code setting that option: logly.options({date: true}) but within the module I used a longer datePrefix. If that feels dirty, you can call it 'date' internally as well instead of 'datePrefix'.

@tristanls
tristanls Aug 23, 2012

thinking about it more, it makes sense, we already have a convention to support two different color spellings, so a datePrefix being different seems consistent to me

@tristanls
Owner

Thank you @brunomorency !

I'm happy to add it. I've added some inline comments for your consideration.

@brunomorency

Great, do you want me to make those changes or do you prefer putting it to your own tastes?

@tristanls
Owner

I'll merge your stuff in and add what we agreed to. (I have to add to documentation, so might as well do it together).

@tristanls tristanls merged commit 4e8e587 into tristanls:master Aug 23, 2012
@tristanls
Owner

@brunomorency all updated, committed, and published. If you have a chance, please take a look if I messed something up. Cheers!

@brunomorency

Changes made post-merge seem good to me, thanks for quickly accepting that pull request!

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