Doc custom #124

bmeck · 2016-10-06T16:24:19Z

Superseding : #123
Dropped for: #125 #127

Documents custom tokens and formatters a bit more/makes the API public knowledge in order to keep it stable in future. Also adds a few tests. Used *Each in tests because after doesn't work as expected.

dougwilson · 2016-10-06T22:35:41Z

Thanks, @bmeck . Looks like there is a lot going on in this PR than just a docs change, so will need some time to look through it.

dougwilson · 2016-10-06T23:17:50Z

README.md

@@ -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 


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.

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

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

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.

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.

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

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.

@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.

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.

dougwilson · 2016-10-06T23:18:31Z

README.md

+var morgan = require('morgan')
+
+morgan.token('echo', function getId (req, res, arg) {
+  return arg


The examples should be something someone will actually do in the real world. Please re-work the example to something a user would actually do.

moved to header[...]

dougwilson · 2016-10-06T23:19:28Z

README.md

+var JSONOutput = function (morgan, req, res) {
+  return JSON.stringify({
+    status: morgan.status(req,res),
+    default: combined(morgan, req, res),


I'm confused; would someone do this in a real app? Not sure why someone would do this if you can elaborate on that.

we do it currently so that we can use JSON metadata queries on the logs.

Is it possible to use another variable than combined since it's not the combined format? Also, can you use a named function instead of an anonymous function? Or you can use an anonymous function directly in the morgan() call.

changing to formatter

dougwilson · 2016-10-06T23:21:58Z

test/morgan.js

@@ -822,6 +822,79 @@ describe('morgan()', function () {
        test.write('0')
      })
    })
+
+    describe('a custom', function () {


I'm not sure I understand the description. This right now reads in the unit tests "morgan() tokens a custom" ??

I tried to match the a function in the same file, can rename

Sure, but "a" is an article and "function" in a noun, so it makes sense. I assume that you're not referring to "custom" the noun here, though you're using it as a noun. I guess it sounds weird since the string doesn't end in the noun you are trying to mean.

changed to a custom token function provided to .token(name,fn)

Cool. I don't doubt you changed the name, but I'm also not seeing a test called "a custom token function provided to .token(name,fn)"? What line is that on?

dougwilson · 2016-10-06T23:24:07Z

test/morgan.js

+      })
+
+      it('should overwrite existing', function (done) {
+        assert.equal(morgan.url, token)


Hm. I know that is ends up as a property on morgan, but I'm not sure if I want to really promise that functionality... Will have to think on this.

would need to change the API contract of the arguments of morgan() but seems doable.

I'm talking about what we promise from an API. Please change to describe the promises we're trying to make here.

what is a condoned way to check this...

There is no public API to get tokens except the tokens argument from within a formatter. We can add an API to do this, if we think there is a use-case around having that.

dougwilson · 2016-10-06T23:28:52Z

test/morgan.js

+      var url
+      beforeEach(function () {
+        url = morgan.url
+        morgan.token('url', token)


Why even bother overwriting an existing token? Are you testing the ability to overwrite already-existing tokens? That seems like it should be a different test, as these tests seem to be trying to test a custom token function, in which case, defining a token with a new name would be a lot clearer in the below tests.

because thats what the docs say happens.

Then you need different tests.

i am writing tests against the docs, unclear why I need different ones.

Hm. I've been thinking on the "unclear why I need different ones" and I feel like I described it above, but obviously we have a disconnect here.. not sure how to proceed. Thoughts?

dougwilson · 2016-10-06T23:34:54Z

test/morgan.js

+        done()
+      })
+
+      it('should use - if result is falsey', function (done) {


I think the test description is weird here (for reference, it is: "morgan() tokens a custom should use - if result is falsey"; result of what? use it where?). Assuming the description above this is changed, this it should maybe something along the lines of "should use "-" as the token's value"--probably need to write somewhere that that is when the token returns "undefined" if you want to test falsy, you should really add more tests that guarantee that, like probably one for false, null, 0, and '', which are all very interesting.

Since the interface is said to return a string, you may be able to skip the false and 0 I guess.

the interface returns an any and uses returnValue || '-' in

morgan/index.js

Line 387 in 1533cdc

return '" +\n (' + tokenFunction + '(' + tokenArguments + ') || "-") + "'

it is not only a string return value, returning "" also invokes this behavior.

Then add all the other tests.

dougwilson · 2016-10-06T23:36:24Z

test/morgan.js

+        .expect(200, cb)
+      })
+
+      it('should not see empty brackets', function (done) {


What should not see the empty brackets? I feel like this test doesn't belong in this describe at all; it seems to be testing the parsing of the format string itself, not the functioning of the token itself?

the token function doesn't see empty backets, it gets no argument, and the [] is just added to the output string.

But that is a test of the format parsing, not the token itself; doesn't belong in this describe.

moved the custom (not predefined) token tests to its own top level describe

dougwilson · 2016-10-06T23:37:07Z

test/morgan.js

+        })
+
+        var server = createServer(':url[HIDDEN]', { stream: stream }, function () {
+          test.abort()


I don't understand the propose of the request abort here. Can you elaborate on that?

it was copy paste, unclear why its in other tests, so I left it.

Then please remove it from this test. The only other test it's in are specifically testing how the token's logic is reacting to aborted requests.

dougwilson · 2016-10-06T23:38:54Z

README.md

+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 })


Use a more realistic example, please :)

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

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

fixed, example moving to header[...]

I'm not seeing the fix reflected here?

dougwilson · 2016-10-07T01:45:15Z

test/morgan.js

+      '',
+      null,
+      undefined
+    ].forEach(function (v) {


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.

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

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.

dougwilson · 2016-10-07T01:45:38Z

test/morgan.js

+      undefined
+    ].forEach(function (v) {
+      it('for ' + (String(v) || JSON.stringify(v)), function (done) {
+        test(v, done)


Please in-DRY this as well.

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

dougwilson · 2016-10-07T01:46:32Z

test/morgan.js

+      next()
+    })
+
+    request(server).post('/test').query('foo=bar').expect(200, cb)


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

dougwilson · 2016-10-07T01:47:13Z

test/morgan.js

+      cb(null, null, line)
+    })
+
+    var server = createServer(':urlpart[query]', { stream: stream }, function (req, res, next) {


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

dougwilson · 2016-10-07T01:49:41Z

test/morgan.js

+    .expect(200, cb)
+  })
+
+  it('should use the result', function (done) {


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?

dougwilson · 2016-10-07T01:50:50Z

test/morgan.js

+  it('should not see empty brackets as an argument value', function (done) {
+    morgan.token('checkempty', function ret (req, res, arg) {
+      assert.equal(arg, undefined)
+      assert.equal(arguments.length, 2)


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

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

dougwilson · 2016-10-07T01:51:38Z

test/morgan.js

+
+  it('should not see empty brackets as an argument value', function (done) {
+    morgan.token('checkempty', function ret (req, res, arg) {
+      assert.equal(arg, undefined)


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

dougwilson · 2016-10-07T01:53:02Z

test/morgan.js

+    })
+  })
+
+  it('should not see empty brackets as an argument value', function (done) {


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?

dougwilson · 2016-10-07T01:54:17Z

test/morgan.js

+    done()
+  })
+
+  describe('should use the string `-` if return value of the token function is falsey', function () {


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.

dougwilson · 2016-10-07T01:56:31Z

README.md

@@ -310,6 +325,54 @@ function assignId (req, res, next) {
 }
 ```

+#### arguments to custom token formats


usually all the examples are just at the same heading level

dougwilson · 2016-10-07T01:57:14Z

README.md

+var express = require('express')
+var morgan = require('morgan')
+
+morgan.token('header', function getId (req, res, arg) {


I think the getId still being here is a typo.

dougwilson · 2016-10-07T01:58:18Z

README.md

+var morgan = require('morgan')
+
+morgan.token('header', function getId (req, res, arg) {
+  return String(req.headers[arg])


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.

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

dougwilson · 2016-10-07T02:00:15Z

README.md

@@ -310,6 +325,54 @@ function assignId (req, res, next) {
 }
 ```

+#### arguments to custom token formats
+
+Example of a custom token format that uses an argument. It will output the


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.

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!'); })

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

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?

@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!') })

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.

Added this to #125

dougwilson · 2016-10-07T02:03:15Z

README.md

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


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

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).

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

dougwilson · 2016-10-07T02:07:00Z

I'm not sure what is happening and where some of your comments are disappearing to; there is so much happening and every email for your comments simply link to https://github.com/expressjs/morgan/pull/124 which is not helpful. Please forgive me if I'm not responding to everything, because some of your comments I just cannot find to respond to.

bmeck · 2016-10-07T03:03:49Z

Gonna just take a step back and probably bail on this, seems a lot of pushback.

dougwilson · 2016-10-07T03:06:10Z

Sorry, reading back my comment I see what you mean. The "not helpful" was a comment to GitHub, in that the emails they are sending me are not helping me locate where your comment is in order for me to actually reply; it was not directed at you, I'm sorry if it came off that way :(

indexzero · 2016-10-07T04:38:00Z

@dougwilson took another stab at this on #125

bmeck · 2016-10-11T13:44:16Z

@dougwilson split into #125 and #127

dougwilson · 2016-10-12T03:09:06Z

Thanks you two; I have taken a look at both PRs now. It seems like some of the review comments from this review got lost in the shuffle to new reviews, so if you re-read through here and see any that apply to your PR, that would help a lot! Otherwise I'll spend some time over this week cherry picking the ones that still apply :)

bmeck force-pushed the doc-custom branch from 5498d1c to ad6cf57 Compare October 6, 2016 16:25

Document custom function for tokens/format

05d4d1c

bmeck force-pushed the doc-custom branch from ad6cf57 to 05d4d1c Compare October 6, 2016 16:33

dougwilson self-assigned this Oct 6, 2016

dougwilson added the pr label Oct 6, 2016

bmeck mentioned this pull request Oct 6, 2016

[do not merge] Add ability to generate JSON by passing an Object as the format #123

Closed

dougwilson suggested changes Oct 6, 2016

View reviewed changes

dougwilson added docs tests labels Oct 6, 2016

bmeck added 3 commits October 6, 2016 20:14

fixup

c912b90

more fixup

ad51d1c

move some of the token tests to top level describe

12230b2

dougwilson reviewed Oct 7, 2016

View reviewed changes

indexzero mentioned this pull request Oct 7, 2016

Rework documentation around formats and custom format functions. #125

Closed

bmeck mentioned this pull request Oct 11, 2016

make tests for custom token() usage #127

Closed

bmeck closed this Oct 11, 2016

Doc custom #124

Doc custom #124

Conversation

bmeck commented Oct 6, 2016 • edited Loading

dougwilson commented Oct 6, 2016

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

bmeck commented Oct 6, 2016 •

edited

Loading