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

Doc custom #124

Closed
wants to merge 4 commits into from
Closed
Show file tree
Hide file tree
Changes from all commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
63 changes: 63 additions & 0 deletions README.md
Original file line number Diff line number Diff line change
Expand Up @@ -22,6 +22,8 @@ Create a new morgan logger middleware function using the given `format` and `opt
The `format` argument may be a string of a predefined name (see below for the names),
a string of a format string, or a function that will produce a log entry.

For the syntax of strings and functions see [morgan.compile](#morgancompileformat).

#### Options

Morgan accepts these properties in the options object.
Expand Down Expand Up @@ -103,6 +105,14 @@ To define a token, simply invoke `morgan.token()` with the name and a callback f
morgan.token('type', function (req, res) { return req.headers['content-type'] })
```

Tokens can accept a string argument passed in from `[]` brackets by specifying
a third argument `arg`:
```js
morgan.token('type', function (req, res, arg) { return arg })
Copy link
Contributor

Choose a reason for hiding this comment

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

Use a more realistic example, please :)

Copy link
Author

Choose a reason for hiding this comment

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

hard to think of a real example without a large example.

Copy link
Contributor

Choose a reason for hiding this comment

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

I can put some thought into it over the weekend if you're not able to think of anything, no problem :)

Copy link
Author

Choose a reason for hiding this comment

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

fixed, example moving to header[...]

Copy link
Contributor

Choose a reason for hiding this comment

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

I'm not seeing the fix reflected here?

```

Falsey values returned from this function will be replaced with `-`.

Calling `morgan.token()` using the same name as an existing token will overwrite that token definition.

##### :date[format]
Expand Down Expand Up @@ -178,6 +188,11 @@ be passed using `[]`, for example: `:token-name[pretty]` would pass the string
Normally formats are defined using `morgan.format(name, format)`, but for certain
advanced uses, this compile function is directly available.

The function returned takes arguments `tokens` , `req`, and `res` where `tokens`
refers to `morgan` itself. If a log should be skipped the function will return
`null`. The function returned or a custom function can be passed directly to
Copy link
Contributor

Choose a reason for hiding this comment

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

I think adding it somewhere else is a lot clearer; adding the information for the function that you pass into the first argument of morgan way down here seems too far out of the way to me.

Copy link
Author

Choose a reason for hiding this comment

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

given the existing format of docs I felt this was the appropriate place, can hoist to top?

Copy link
Author

Choose a reason for hiding this comment

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

should we move the description of the string/id to the top as well?

Copy link
Contributor

Choose a reason for hiding this comment

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

I guess

Copy link
Contributor

Choose a reason for hiding this comment

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

I still think it feels like you just stuck this in a random spot (when reading the docs--why do I need to read the .compile() function to understand what to pass into format? The current docs direct to https://github.com/expressjs/morgan#predefined-formats not the compile() function.

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 just make different PRs? Perhaps at least separate the tests additions from the docs additions? I'm losing track of what is happening and what is changing without needing to try and re-read through everything over and over, and there is a lot of things.

Copy link
Contributor

Choose a reason for hiding this comment

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

The location makes sense to me. It is documenting the capabilities of the functions returned by morgan.compile

Copy link
Contributor

Choose a reason for hiding this comment

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

Ah. So maybe I'm just confused here. I was under impression from the issue that spawned this PR that the goal here was to document what the format argument in morgan(format, options) accepts. Are we missing that documentation and this is the documentation for morgan.compile? Since I mistook this as the documentation for the format argument, my thought here was that documentation for the format argument should be up in the section regarding the format argument, rather than in the morgan.compile section.

Copy link
Contributor

Choose a reason for hiding this comment

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

@dougwilson it is about documenting the format argument in morgan(format, options). On line 25 of README.md in this PR @bmeck added the phrase:

For the syntax of strings and functions see morgan.compile.

He then added this documentation regarding the semantics of functions returned by morgan.compile which is what is linked to by the documentation on line 25.

Copy link
Contributor

Choose a reason for hiding this comment

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

Hm. I don't understand. I think I may just need to step back and process it. I see that change, but when I read through https://github.com/bmeck/morgan/blob/12230b264a41b04255e0d567ace2110276b5e62d/README.md just as a whole it didn't seem clear.

My process was I read the following:

The format argument may be a string of a predefined name (see below for the names), a string of a format string, or a function that will produce a log entry.

Ok, predefined named, I'll see below for what that means. Oh, below it says "For the syntax of strings and functions see morgan.compile."

Ok, click that link.

Compile a format string into a function for use by morgan

Cool.

A format string is a string that represents a single log line and can utilize token syntax. Tokens are references by :token-name. If tokens accept arguments, they can be passed using [], for example: :token-name[pretty] would pass the string 'pretty' as an argument to the token token-name.

Cool. Good to understand about the tokens. I wonder what tokens are? I guess we'll get to that.

Normally formats are defined using morgan.format(name, format), but for certain advanced uses, this compile function is directly available.

Ok, cool. Seems unrelated to the format argument I came to read about, but I can skip this line.

The function returned takes arguments tokens , req, and res where tokens refers to morgan itself. If a log should be skipped the function will return null. The function returned or a custom function can be passed directly to morgan using morgan(myFn).

Cool, so that's what morgan.compile returns. But wait, the section is over. What do I give as the format argument? I know that morgan.compile takes a string as it's format argument, which is some token syntax thing, and that it returns a function that does some stuff.

Oh wait, I guess "The function returned or a custom function can be passed directly to morgan using morgan(myFn)." was just snuck there at the end, and I guess I can make the connection now.

Basically, my critism is that it's too 'round about a way. I'm sorry, I just don't see how this is useful documentation for the goal of just trying to get the format argument to the constructor across. It's just too much of a round about way.

If you strongly disagree, we can always wait for other Express.js members to chime in as well to see what we think collectively, but it just doesn't read well enough for me to want to merge in, personally.

morgan using `morgan(myFn)`.

## Examples

### express/connect
Expand Down Expand Up @@ -310,6 +325,54 @@ function assignId (req, res, next) {
}
```

#### arguments to custom token formats
Copy link
Contributor

Choose a reason for hiding this comment

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

usually all the examples are just at the same heading level


Example of a custom token format that uses an argument. It will output the
Copy link
Contributor

Choose a reason for hiding this comment

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

Do we think it may be confusing for an example showing how to make a token that is already provided by the module? I'm just wondering because there was a module where we showed an example of how to re-implement something that was already in the module and it seemed people kept posting on Stackoverflow with the confusion that that functionality was not actually part of the library and instead just kept copying the example as the answer for how to do that action, rather than using the (actually maintained) built-in method.

Copy link
Contributor

Choose a reason for hiding this comment

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

This is a solid point. I've had similar experiences on other projects. How about a more absurd example such as "is it christmas?"

Example of a custom token format that uses an argument. It will output if the time
of the current request is christmas.

var express = require('express')
var morgan = require('morgan')

morgan.token('is-holiday', function getId (req, res, arg) {
  var date = new Date()
  var shortDate = `${date.getMonth() + 1}/${date.getDate()}`;
  var holiday = arg.toLowerCase().replace(/'/g, '');

  var holidays = {
    '1/1': 'new years day',
    '2/14': 'valentines day',
    '5/5': 'cinco de mayo',
    '10/31': 'halloween',
    '12/25': 'christmas',
    '12/31': 'new years eve'
  };

  var isHoliday = 'Nope';
  if (holidays[shortDate] === holiday) {
    isHoliday = 'Yup';
  }

  return String(isHoliday);
})

var app = express()

app.use(morgan('Is it Christmas? :is-holiday[christmas] :response-time'))

//
// Alternatively we could ask if it is Halloween
//
app.use(morgan('Is it Halloween? :is-holiday[halloween] :response-time'))


app.get('/', function (req, res) {
  res.send('hello, world!');
})

Copy link
Contributor

Choose a reason for hiding this comment

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

I mean, if we are struggling to even come up with a real example at all, then why even make an example?

Copy link
Contributor

Choose a reason for hiding this comment

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

How do you use this feature in your production systems? Just take that use-case (which is clearly real-world) and place it as the example?

Copy link
Contributor

Choose a reason for hiding this comment

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

@dougwilson how about this example which outputs a safe subset of process information?

Example of a custom token format that uses an argument. It will output the
property from process related to the argument passed to the token. The valid
arguments are restricted to safe properties of the process object in Node.js.

var express = require('express')
var morgan = require('morgan')

morgan.token('process', function getId (req, res, arg) {
  var accessors = {
    arch: function () { return process.arch },
    pid: function () { return process.pid },
    platform: function () { return process.platform },
    title: function () { return process.title },
    uptime: function () { return process.uptime() },
    version: function () { return process.version }
  }

  if (accessors[arg]) {
    return String(accessors[arg]());
  }

  return String('Unknown arg: ' + arg);
})

var app = express()

app.use(morgan(':process[pid] :process[uptime] :response-time'))

app.get('/', function (req, res) {
  res.send('hello, world!')
})

Copy link
Contributor

Choose a reason for hiding this comment

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

Yea, that seems cool. Could probably simplify it for the example, especially since based on the design currently, it should not return something like Unknown arg: ..., and should be undefined or something falsy to get turned into a '-'. Probably could simply the example by removing the need for the additional layer of functions, since the format string is not an untrusted string.

morgan.token('process', function (req, res, prop) {
  return process[prop]
}

should be good enough for example purposes.

Copy link
Contributor

Choose a reason for hiding this comment

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

Added this to #125

header related to the argument passed to the token.

```js
var express = require('express')
var morgan = require('morgan')

morgan.token('header', function getId (req, res, arg) {
Copy link
Contributor

Choose a reason for hiding this comment

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

I think the getId still being here is a typo.

return String(req.headers[arg])
Copy link
Contributor

Choose a reason for hiding this comment

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

Does it make sense to show an example of returning an unknown header as the string undefined? If it just returns undefined directly, it shows that unknown values get replaced as '-' in the output.

Copy link
Contributor

Choose a reason for hiding this comment

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

This is moot if we change the example to "Is it Christmas?"

})

var app = express()

app.use(morgan(':header[host] :response-time'))

app.get('/', function (req, res) {
res.send('hello, world!')
})
```

### use custom format function

Sample app that will use a custom formatting function. This will print JSON
instead of a simple string.

```js
var express = require('express')
var morgan = require('morgan')

var formatter = morgan.compile('[:date[clf]] :method :url :res[content-length]')
var JSONOutput = function (morgan, req, res) {
Copy link
Contributor

Choose a reason for hiding this comment

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

The example should document as we want to describe: the first argument is tokens, not morgan.

Copy link
Contributor

Choose a reason for hiding this comment

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

Especially an issue since it's confusing when you're shadowing a variable (there is already a var morgan = above, making it hard for users to sometimes understand what is actually being used here--the global or the argument).

Copy link
Contributor

Choose a reason for hiding this comment

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

I think you're right @dougwilson. This sample should fail if I understand the shadowing correctly.

return JSON.stringify({
status: morgan.status(req,res),
default: formatter(morgan, req, res),
})
}

var app = express()

app.use(morgan(JSONOutput))

app.get('/', function (req, res) {
res.send('hello, world!')
})
```

## License

[MIT](LICENSE)
Expand Down
97 changes: 97 additions & 0 deletions test/morgan.js
Original file line number Diff line number Diff line change
Expand Up @@ -7,6 +7,7 @@ var join = require('path').join
var morgan = require('..')
var request = require('supertest')
var split = require('split')
var url = require('url')

describe('morgan()', function () {
describe('arguments', function () {
Expand Down Expand Up @@ -1316,6 +1317,102 @@ describe('morgan.compile(format)', function () {
})
})

describe('morgan.token(name, function)', function () {
it('should overwrite existing functions if called multiple times', function (done) {
function token (req, res, arg) {
return arg && url.parse(req.url)[arg]
}
var urlToken = morgan.url
morgan.token('url', token)
assert.equal(morgan.url, token)
morgan.token('url', urlToken)
done()
})

describe('should use the string `-` if return value of the token function is falsey', function () {
Copy link
Contributor

Choose a reason for hiding this comment

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

The describe should be keep below the it at the same level, that way they are ordered in the code in the way that mocha outputs it, which makes it so much easier to correlate them from the test output into the file. It's what we try to keep our style as.

function test (v, done) {
morgan.token('ret', function ret (req, res, arg) {
return arg
})
var cb = after(2, function (err, res, line) {
if (err) return done(err)
assert.equal(line, '-')
done()
})

var stream = createLineStream(function (line) {
cb(null, null, line)
})

var server = createServer(':ret', { stream: stream }, function (req, res, next) {
next()
})

request(server)
.get('/')
.expect(200, cb)
}
[
0,
NaN,
false,
'',
null,
undefined
].forEach(function (v) {
Copy link
Contributor

Choose a reason for hiding this comment

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

The pattern I would like to see in the tests is they should not be DRY; I would rather see a separate it for every one of these.

Copy link
Author

Choose a reason for hiding this comment

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

I'm opposed to this as changing one of those tests and not changing other could introduce issues.

Copy link
Contributor

Choose a reason for hiding this comment

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

Sure, but it's just a personal preference, especially the hassle it makes when debugging the tests. I would rather land a PR that matches all the rest of the org style rather than having a snowflake here.

it('for ' + (String(v) || JSON.stringify(v)), function (done) {
test(v, done)
Copy link
Contributor

Choose a reason for hiding this comment

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

Please in-DRY this as well.

Copy link
Author

Choose a reason for hiding this comment

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

see concern about keeping tests in sync if they end up changing.

})
})
})

it('should not see empty brackets as an argument value', function (done) {
Copy link
Contributor

Choose a reason for hiding this comment

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

From the earlier comment, this test is not testing tokens; it's testing the parsing of the format string. i.m.o. the ignoring of the [] can really be considered a bug, not a feature. Perhaps we should be it rather than engrain the bug into the design?

morgan.token('checkempty', function ret (req, res, arg) {
assert.equal(arg, undefined)
Copy link
Contributor

Choose a reason for hiding this comment

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

This would need to be assert.strictEqual if you want to know it's actually undefined.

assert.equal(arguments.length, 2)
Copy link
Contributor

Choose a reason for hiding this comment

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

I don't think it's generally a good practice that we should be guaranteeing the arguments.length.

Copy link
Author

Choose a reason for hiding this comment

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

sounds fine, but I am just testing the actual existing behavior, if we sent the same argument length every time I would prefer that change to removing this test

cb(null)
})
var cb = after(3, function (err, res, line) {
if (err) return done(err)
assert.equal(line, '-[]')
done()
})

var stream = createLineStream(function (line) {
cb(null, null, line)
})

var server = createServer(':checkempty[]', { stream: stream }, function (req, res, next) {
next()
})

request(server)
.get('/')
.expect(200, cb)
})

it('should use the result', function (done) {
Copy link
Contributor

Choose a reason for hiding this comment

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

This test says "morgan.token(name, function) should use the result". I'm not certain what that means. What should use the result? The result of what?

morgan.token('urlpart', function ret (req, res, arg) {
return url.parse(req.url)[arg]
})
var cb = after(2, function (err, res, line) {
if (err) return done(err)
assert.equal(line, 'foo=bar')
done()
})

var stream = createLineStream(function (line) {
cb(null, null, line)
})

var server = createServer(':urlpart[query]', { stream: stream }, function (req, res, next) {
Copy link
Contributor

Choose a reason for hiding this comment

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

It doesn't seem necessary to pas the function if it does nothing.

next()
})

request(server).post('/test').query('foo=bar').expect(200, cb)
Copy link
Contributor

Choose a reason for hiding this comment

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

Usually each of these method calls we put on a new line.

})
})

function after (count, callback) {
var args = new Array(3)
var i = 0
Expand Down