Rework documentation around formats and custom format functions. #125
Conversation
|
This looks a lot better :) I do see there are a few things copied from the other PR that I had feedback on. If you can make the adjustments on those items from the comments I made in the other PR, that would be fabulous :) I do have some comments here, but i have to get to sleep for a weekend trip, but I will be back here Tuesday to see what changed and add my additional comments 👍 |
|
@dougwilson roger that. Did another pass to clarify the four options folks have:
|
README.md
Outdated
|
|
||
| ##### Using a format function [from `morgan.compile`](#morgancompileformat) | ||
| ``` js | ||
| morgan(morgan.compile(':method :url :status :res[content-length] - :response-time ms')) |
There was a problem hiding this comment.
I'm not sure if I want to explain what the difference is between this example and the one above is. I would just leave out this example.
There was a problem hiding this comment.
Sure. Changed this in 1c9416c. Not sure why this Github comment wasn't hidden automatically because the line was changed.
README.md
Outdated
| @@ -20,7 +20,39 @@ var morgan = require('morgan') | |||
|
|
|||
| Create a new morgan logger middleware function using the given `format` and `options`. | |||
| 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. | |||
| a string of a format string, or a format function that will produce a log entry. | |||
There was a problem hiding this comment.
So the wording is in the format "a ". Can we keep that wording for the function as well? Perhaps "or a function that will format a log entry"?
There was a problem hiding this comment.
Sure. Changed this in 1c9416c. Not sure why this Github comment wasn't hidden automatically because the line was changed.
README.md
Outdated
| a string of a format string, or a function that will produce a log entry. | ||
| a string of a format string, or a format function that will produce a log entry. | ||
|
|
||
| Format functions accept three arguments `tokens`, `req`, and `res` where `tokens` represents |
There was a problem hiding this comment.
And then probably start this with "The function will be called with three arguments: [...]"
There was a problem hiding this comment.
Sure. Changed this in 1c9416c. Not sure why this Github comment wasn't hidden automatically because the line was changed.
README.md
Outdated
| an object with all known tokens. It is expected to return a string. For the sake of example, the | ||
| following four code snippets are equivalent. | ||
|
|
||
| ##### Using a [predefined format string](#predefined-formats) |
There was a problem hiding this comment.
I think this skipped a level of heading by accident.
There was a problem hiding this comment.
Sure. Changed this in 1c9416c. Not sure why this Github comment wasn't hidden automatically because the line was changed.
README.md
Outdated
| Calling `morgan.token()` using the same name as an existing token will overwrite that token definition. | ||
| There are a few pieces of behavior you should be aware of when using tokens: | ||
|
|
||
| - `morgan.token('name', callback)` corresponds to `:name` in any string passed to `morgan(format, options)`. |
There was a problem hiding this comment.
In the Express.js readmes, we usually format the bullet lists as "<space><space>*<space>"
There was a problem hiding this comment.
Also, let's not call the function callback, as that has a significance in the Node.js land, and this is not a function that is in the form of a callback (it's sync and isn't called with err as the first argument).
There was a problem hiding this comment.
Sure. Changed this in 1c9416c. Not sure why this Github comment wasn't hidden automatically because the line was changed.
README.md
Outdated
|
|
||
| morgan.token('process', function getId (req, res, arg) { | ||
| if (typeof process[arg] === 'function') { | ||
| return String(process[arg]()) |
There was a problem hiding this comment.
I think this line misses the functionality for taking advantage of the built-in falsy behavior,
There was a problem hiding this comment.
Sure. Changed this in 1c9416c. Not sure why this Github comment wasn't hidden automatically because the line was changed.
README.md
Outdated
| var morgan = require('morgan') | ||
|
|
||
| var route = morgan.compile(':method :url') | ||
| var outputJson = function (tokens, req, res) { |
There was a problem hiding this comment.
Why not used a named function?
There was a problem hiding this comment.
Sure. Changed this in 1c9416c. Not sure why this Github comment wasn't hidden automatically because the line was changed.
README.md
Outdated
| var outputJson = function (tokens, req, res) { | ||
| return JSON.stringify({ | ||
| route: route(tokens, req, res), | ||
| status: morgan.status(req, res), |
There was a problem hiding this comment.
I think this a typo: "morgan" should probably be "tokens" on this line.
There was a problem hiding this comment.
Sure. Changed this in 1c9416c. Not sure why this Github comment wasn't hidden automatically because the line was changed.
README.md
Outdated
| return JSON.stringify({ | ||
| route: route(tokens, req, res), | ||
| status: morgan.status(req, res), | ||
| 'content-length': morgan.res(req, res, 'content-length') |
There was a problem hiding this comment.
Same morgan -> tokens typo on this line.
There was a problem hiding this comment.
Sure. Changed this in 1c9416c. Not sure why this Github comment wasn't hidden automatically because the line was changed.
README.md
Outdated
| A request to `GET /` would cause `morgan` to log the following JSON: | ||
|
|
||
| ``` json | ||
| { "route": "GET /", "status": 200, "content-length": 13 } |
There was a problem hiding this comment.
So we don't have any examples of the output for any other example apps above. If we include an example, shouldn't it at least be exactly what the user would see? I don't think JSON.stringify includes those space characters.
There was a problem hiding this comment.
Sure. Changed this in 1c9416c. Not sure why this Github comment wasn't hidden automatically because the line was changed.
|
I will make the changes you requested, but I'd like to point out that this has been far more difficult than I expected. I almost just gave up. One tactic I've used to encourage contribution when I have very particular requirements about nuance is to simply merge the PR and clean it up myself. You may think this is more of a time suck but in my experience I've found it's as time consuming as giving a detailed PR review as you have here.So the net-net time is the same. |
|
It's a difficult balance. I used to do that post merge clean up, but I would consistently get angry people, accusing me of not merging what they wanted. I just gave up on that, because it was not a happy place to be in where I thought I was helping but getting yelled at. If you are giving me permission to do that, I can do that, though. Hopefully you'll agree with the changes I make, though, because I have had a consistently bad experience doing what you are asking me to do. |
|
@dougwilson made all the changes you asked for. At this point I'd simply like to see this land. Feel free to make any future edits post-merge. |
|
@dougwilson bump. |
|
Hi @indexzero, I haven't forgotten about your PR. I just have to make those "post-merge" edits, but have not been at a computer very much in order to do this. I'll take care of this as soon as I can. |
|
Thanks for following up and cherry-picking this. |
@dougwilson thanks for the feedback on #124. I've reworded what @bmeck wrote incorporating your feedback. Hopefully this will communicate the material more clearly.
📚 📘 Read the rendered Markdown for README.md 📖 📚