clarkie / dynogels

DynamoDB data mapper for node.js. Originally forked from https://github.com/ryanfitz/vogels
Other
490 stars 110 forks source link

Remove logging #81

Closed focusaurus closed 7 years ago

focusaurus commented 7 years ago

Proposing this as one approach to fix #65. This is a breaking semver major change but this is what I prefer as I prefer logging to only happen in application code, not in library code, and in particular bunyan's use of native modules that are incompatible between node v4 and node v6 is really annoying. I'm proposing just remove logging entirely, the other options include:

coveralls commented 7 years ago

Coverage Status

Coverage increased (+0.06%) to 98.726% when pulling 5105b33225a8f0b7fc9b9a714e8c66081e1e1e57 on focusaurus:logging into 934d101501a7f507c030813fbcbba56b2ffc815d on clarkie:master.

cludden commented 7 years ago

I absolutely agree with your thinking. However, to avoid the breaking changes, I think I would be in favor of implementing option 2 (Allow clients to plug in a logging function, then adapt the arguments to their logger as necessary) or 3 (Emit loggable events instead such as info, error, warn, etc), similar to how the elasticsearch.js client works, but maybe with a slightly more elegant API... thoughts?

clarkie commented 7 years ago

I'm with @cludden it would be nice to allow consumers to pass in their own logging library. I might have a go at this today as I'm looking at using dynogels in lambda too.

focusaurus commented 7 years ago

Also just to note so we're clear, I'd like at some point for the bunyan dependency to be removed. When that happens, I can't see a way to make that non-breaking, so I think eventually a semver major will be required around logging.

clarkie commented 7 years ago

Ah yeah, sure. That's the plan, but we could leave an example where bunyan is passed in an optional argument or something

clarkie commented 7 years ago

@cludden can you have a look at #84, thanks