pinojs / quick-format-unescaped

Solves a problem with util.format
MIT License
17 stars 13 forks source link

No format string => no formatting #27

Closed jsumners closed 4 years ago

jsumners commented 4 years ago

The following makes no sense because there are not any format identifiers in the format string:

format('foo', ['bar']) // => 'foo bar'

The following doesn't make any sense because invoking a function is not the same thing as using the apply prototype method:

format(['%s', 'foo']) // => 'foo'

This PR removes these nonsense use cases such that:

  1. yields: 'foo'
  2. yields: null

This PR also makes the behavior match the documentation. No where in the docs does it enumerate the currently implemented surprising behaviors.

If this PR is accepted it will resolve https://github.com/pinojs/pino/issues/793

This PR will clearly necessitate a semver major release.

jsumners commented 4 years ago

ping: @davidmarkclements @mcollina

jsumners commented 4 years ago

Well it doesn't exactly match console.log:

❯ node
Welcome to Node.js v12.14.1.
Type ".help" for more information.
> console.log('foo', ['bar'])
foo [ 'bar' ]
undefined
> console.log('foo', [{}])
foo [ {} ]
undefined
>

Nor does it match Winston (https://runkit.com/embed/n0w7euy6ewl4):

{\"level\":\"info\",\"message\":\"foo\"}"

But it does match the odd man out, Bunyan:

❯ node
Welcome to Node.js v12.14.1.
Type ".help" for more information.
> var log = require('bunyan').createLogger({name:'logger'})
undefined
> log.info('foo', {})
{"name":"logger","hostname":"Jamess-MBP","pid":74872,"level":30,"msg":"foo {}","time":"2020-03-20T23:02:24.599Z","v":0}
undefined
>

I argue that this just makes no sense. No format tokens have been passed. Thus, no formatting should occur.

jsumners commented 4 years ago

Are you suggesting a semver patch PR to add documentation for the surprising behavior?

mcollina commented 4 years ago

I guess I’m asking if this PR matches the behavior documented in the readme.

jsumners commented 4 years ago

It’s does.

jsumners commented 4 years ago

@mcollina can you merge and release this and #28? Or do we need to wait on @davidmarkclements?

mcollina commented 4 years ago

I'll wait for Dave if possible.

jsumners commented 4 years ago

😞 I hope he responds soon. I'm blocked on this with no way of patching around it since this module is invoked in the LOG function and it's directly referenced instead of used as a pluggable module (e.g. format(...) instead of this[formatterSym](...)).

davidmarkclements commented 4 years ago

v4.0.0 released

jsumners commented 4 years ago

Thank you very much @davidmarkclements !