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

Match colorize output with Morgan #154

Open
wants to merge 1 commit into
base: master
Choose a base branch
from

Conversation

Hilzu
Copy link

@Hilzu Hilzu commented Aug 1, 2017

Morgan only colors the status code in the log message according to code, documentation and quick testing. This PR makes the log output when using the colorize option to match it. IMHO this also makes the output a lot more readable because the current setting of grey on black has really low contrast with many color themes.

Morgan only colors the status code in the log message:
https://github.com/expressjs/morgan/blob/master/index.js#L201 According
to the documentation of colorize the output should match that of morgan.
@rosston
Copy link
Collaborator

rosston commented Aug 9, 2017

Awesome, thanks for contributing! I'm happy with this change since it improves contrast and readability.

The only thing I'm worried about is that this is technically a breaking change. I'd love to merge it soon, but I have some more breaking changes in mind for express-winston v3. I think I'm going to hold off for now, but I may end up changing my mind later just to get this in. 🙂

@Hilzu
Copy link
Author

Hilzu commented Aug 10, 2017

Would you also consider matching expressFormat with morgan in v3? Currently it matches the morgan dev format but it's missing content length from the end. I can create a PR if you agree that it should be done.

@rosston
Copy link
Collaborator

rosston commented Sep 8, 2017

@Hilzu Sorry, responding very late.

I'm torn on adding the content length to be on by default with expressFormat. I started writing a longer response but ended up deleting it (that's how torn I am).

I'm good with adding content length to expressFormat, but I'm inclined to hold off on that until a v3 release as well. I know it's kind of pedantic, but I don't really want to mess with the log formats of people's files without a major version bump.

@jblevins1991
Copy link

This is a very old PR. What are we doing with this?

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants