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

Move standard formatters to public export #34

Open
rooftopsparrow opened this issue Jan 5, 2017 · 3 comments
Open

Move standard formatters to public export #34

rooftopsparrow opened this issue Jan 5, 2017 · 3 comments

Comments

@rooftopsparrow
Copy link

Hi @TomFrost and contributors,

Would you be open to moving including the formatters included with the module the top level export ? One example for the reasoning behind this request:

The json formatter is almost 100% what I want for everything, but I'd like to add static properties to a particular target ( not globally ). To achieve that, I can modify the function arguments before invoking the standard json function.

I currently can do this just by just reaching into the module ( require('bristol/lib/formatters/json') ) but its brittle because you can change that path an any time 😄

Thanks for the module 👍

@TomFrost
Copy link
Owner

TomFrost commented Jan 9, 2017

Hey Jonathan! Killer suggestion.

Bristol has a stated goal of lazy-loading everything possible, so that leaves us with two options toward that goal:

  1. Define a getter. Either a literal ES6 getter where accessing Bristol.formatters.human would cause that formatter to be require()d in real time, or just a function call like Bristol.getFormatter('human').

  2. Consider the paths of formatters and targets to be versioned entities, so if the path ever needs to change, it will cause a major version change.

I like 2 for its simplicity, but 1 gives you that peace-of-mind guarantee since it's in the code. Any thoughts? :)

Also: Perhaps a better option here would be to make it possible to set "globals" just on one particular target? I'm working on a revamp of how targets are stored and configured, so this could be a very easy feature add if there aren't a bunch of other use cases where exposing the built-in formatters is still the better option.

@MarkHerhold
Copy link
Contributor

I also modified the JSON formatter for my use and I created PR #22 so I didn't have to duplicate the Bristol helper code. I then just created my own JSON formatter based on Bristol's and used the original Bristol utils.

This worked out well for me and I don't have to rely on paths and I don't have to monkey patch JSON formatter code.

@rooftopsparrow
Copy link
Author

rooftopsparrow commented Jan 9, 2017

Hi @TomFrost and @MarkHerhold 👋,

I don't mind requiring the formatter directly, and its pretty standard practice that I see around the community. I'm totally ok with option 2 if you would consider those to be versioned paths 👍

As far as "globals" per targets are concerned -- I think that would be a great feature because most of the time one log format requires some static attributes ( Looking at you Logstash ) than other simpler targets where those globals are just noise.

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

No branches or pull requests

3 participants