search

id0Sch / log4js-json-layout

provides a slim and easy to use json-layout for log4js-node
MIT License
17 stars 8 forks source link

ES6 rewrite and optional colors #2

Closed fluxsauce closed 7 years ago

fluxsauce commented 7 years ago

Howdy, this is a great project! I wanted to use winston on a project but my organization is pretty entrenched in log4js, so this was a perfect compromise.

However, what it didn't support was colors. While you'd never want colors in logs that you're going to be parsing, it is helpful for humans. I added it as an option.

I also performed an ES6 rewrite, added coding standards, found and fixed a couple minor bugs, and hit 100% test coverage and no ESLint problems.

Since the changes are pretty radical, I marked it as a major version. Functionally, it should be the same as before.

Thoughts? I'm happy to adjust as needed.

id0Sch commented 7 years ago

Hi, Thanks for taking the time to maintain this project, It's been a long time since I last made changes and It's great to see the project evolving. Before I go over the changes, Can you please revert the version change? after I'll merge your changes I'll bump it, create a tag and publish the update to npm, i'd like to just release 2.0.0 and not 2.0.1 :)

And as far for the colors, In my organization, we use the regular log4js layout when using the code in dev environment, and switch over to this layout when we're in production - the logs are in json so we pipe stdout to a log collector. I'd really like to avoid adding colors here, as the output of this lib is not meant to be "human-readable". WDYT?

id0Sch commented 7 years ago

The code looks great! let's just sort out the colors things and i'll merge. ps. I added travi.ci integration , can you please add this to the readme?

[![Build Status](https://travis-ci.org/id0Sch/log4js-json-layout.svg?branch=master)](https://travis-ci.org/id0Sch/log4js-json-layout)
fluxsauce commented 7 years ago

Thanks for the feedback! I've made the changes to the version and added the Travis badge.

I'd like to advocate for the color support for development purposes only for a few reasons:

I've also updated the documentation to be more explicit about the purpose and to stress that it's off by default.

Is that a fair compromise? Thanks for consideration.

id0Sch commented 7 years ago

pushed version 2.0.0 to npm. Thanks for your time and effort!