Skip to content
This repository was archived by the owner on Dec 11, 2024. It is now read-only.

Conversation

@boneskull
Copy link
Contributor

Adds several formatting functions and respective configuration options.

  • pedantic: End your messages with a . or user-specified string
  • capitalize: Begin your message with a capital letter
  • normalize: A shortcut for pedantic: true and capitalize: true
  • formatter: User-defined function to reformat any message

I think these are OK, because while the module must be 100% bunyan-compatible, it can be a superset, correct?

My only other concern is index.js:427 which is a pretty flimsy attempt to detect an object or array converted into JSON. The alternative is to attempt to JSON.parse() it, but that's a fair bit of overhead for a "nice-to-have" feature.

Anyway, if you don't like and/or cannot use these changes, is it possible to at least retain Logger.prototype._formatMessage as an "abstract" (empty) function + its invocation, so that I can subclass Logger?

tests + docs

- `pedantic`: End your messages with a `.` or user-specified string
- `capitalize`: Begin your message with a capital letter
- `normalize`: A shortcut for `pedantic: true` and `capitalize: true`
- `formatter`: User-defined function to reformat any message

I *think* these are OK, because while the module must be 100% bunyan-compatible, it *can* be a superset, correct?

My only other concern is `index.js:427` which is a pretty flimsy attempt to detect an object or array converted into JSON.  The alternative is to attempt to `JSON.parse()` it, but that's a fair bit of overhead for a "nice-to-have" feature.

Anyway, if you don't like and/or cannot use these changes, is it possible to at least retain `Logger.prototype._formatMessage` as an "abstract" (empty) function + its invocation, so that I can subclass `Logger`?

tests + docs
@tmpfs
Copy link
Collaborator

tmpfs commented Sep 22, 2014

Hey man, whilst I like the idea and this has crossed my mind a couple of times, this isn't going to make it in like that, mainly that regexp test.

Rather than so many configuration properties, maybe we can have a single format configuration function that is invoked when defined and we expose Logger.formats which defines format functions that behave the same as your descriptions.

The default format function would just call util.format() as it already does in write().

We would then pass the message, parameters, logger instance, etc and the implementation can then call util.format() and perform capitalization etc etc.

Work for you?

@boneskull
Copy link
Contributor Author

I'm sure that'd work for me, but I don't quite understand what you're proposing here:

Rather than so many configuration properties, maybe we can have a single format configuration function that is invoked when defined and we expose Logger.formats which defines format functions that behave the same as your descriptions.

This sounds like you need to actually do something like require('cli-logger').Logger.pedantic to get the reference to the pedantic function. If that's what you meant, this seems rather awkward, especially if you are not using cli-logger directly. This is my use case, and what prompted the PR.

How about an array instead?

// in config
{
  format: ['pedantic', 'capitalize', function custom1(msg) {}, function custom2(msg) {}]
}

Instead of Logger.pedantic, etc., how about

Logger.formatters = { 
  pedantic: function() {}, 
  capitalize: function() {}
  // etc.
};

Given this array (or perhaps support singular string/function values as well), if a string is found, look for equivalent key in the Logger.formatters object. If present, execute the function; otherwise ignore. Process the array in order. If a function is found within the array, just execute it. Like middleware, right?

Re: the JSON thing; perhaps we can flip a switch to say "this message is stringified", when it happens. But perhaps we don't need the check at all. What say you?

@tmpfs
Copy link
Collaborator

tmpfs commented Sep 22, 2014

We can solve that particular issue by allowing conf.formatter to be a string function name if it exists in the list of predefined formatters it is used, otherwise the default formatter is used.

Allowing the format object to be an array is also useful (multiple formatters), although I wonder if it is necessary, surely whatever formatting you desire can be achieved with a single function?

@boneskull
Copy link
Contributor Author

normalize is the one I want to use, but that's not particularly helpful if you want to specify a second parameter to cli-utils/pedantic(). I want this in addition to--not instead of--util.format(). I wrote the PR for a general solution instead of my one use case.

So if the users wants to use pedantic with a second parameter, and make the string captialized, then he/she is SOL unless:

  1. we support multiple formatters
  2. the user has to npm install --save cli-logger, then writes some gnarly function like:
  function(msg, parameters) {
    var Logger = require('cli-logger').Logger,
      m = Logger.capitalize(Logger.pedantic(msg, '!'));
    return require('util').format.apply(null, [m].concat(parameters));
  }
  1. moar config options

@tmpfs
Copy link
Collaborator

tmpfs commented Sep 22, 2014

Ok cool, I think I know what you need, let me think about this a little, busy with commercial work today ;)

@boneskull
Copy link
Contributor Author

Sounds good.

@tmpfs
Copy link
Collaborator

tmpfs commented Sep 22, 2014

FYI, this commit:

cli-kit/cli-command@68b998a

Tidies the module exports such that you can access it via logger on cli-command. Exceptions are cli-rc (optional dependency), markzero and moment.

Apart from those exceptions all dependent modules are exposed as well as various classes from cli-define:

https://github.com/freeformsystems/cli-command/blob/master/index.js#L558

@tmpfs
Copy link
Collaborator

tmpfs commented Sep 23, 2014

I prefer this approach:

2c35594

A formatter function or string formatter name. Supported names are pedantic, capitalize and normalize. Signature is function(record, parameters, format); record is the log record about to be written, parameters is the message replacement parameters and format is the default formatter function bound to the logger instance. Formatter functions are invoked in the scope of the logger.

Does that solve your use case?

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

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants