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

[WIP] Plain ndjson #58

Merged
merged 34 commits into from Apr 28, 2019

Conversation

@jsumners
Copy link
Contributor

jsumners commented Mar 30, 2019

This PR will refactor pino-pretty to be a generic ndjson prettifier. It will recognize various properties that are common in ndjson log lines, and handle those properties in a sane way, but it will no longer skip JSON lines it does not recognize. Instead, it will apply a basic format to them.

TODO:

  • Figure out what format bog standard JSON lines should be presented in (I do not have time for this)
  • Clean up the mess that is object and errors formatting
  • Write some tests for the new utility functions
  • Docs!

For the formatting of standard JSON lines I'm thinking:

{"foo":"bar"}

=>

{
  foo: "bar"
}

and

["foo","bar"]

=>

[
  "foo",
  "bar"
]

Opinions @pinojs/core?

jsumners added 2 commits Mar 30, 2019
@jsumners

This comment has been minimized.

Copy link
Contributor Author

jsumners commented Mar 30, 2019

PS: PRs to this branch are welcome if you see something you want to tackle.

@coveralls

This comment has been minimized.

Copy link

coveralls commented Mar 30, 2019

Pull Request Test Coverage Report for Build 135

  • 75 of 75 (100.0%) changed or added relevant lines in 3 files are covered.
  • No unchanged relevant lines lost coverage.
  • Overall coverage decreased (-1.0%) to 96.939%

Totals Coverage Status
Change from base Build 123: -1.0%
Covered Lines: 167
Relevant Lines: 167

💛 - Coveralls
@coveralls

This comment has been minimized.

Copy link

coveralls commented Mar 30, 2019

Pull Request Test Coverage Report for Build 194

  • 147 of 147 (100.0%) changed or added relevant lines in 3 files are covered.
  • No unchanged relevant lines lost coverage.
  • Overall coverage increased (+1.4%) to 99.415%

Totals Coverage Status
Change from base Build 187: 1.4%
Covered Lines: 193
Relevant Lines: 193

💛 - Coveralls
@jsumners jsumners referenced this pull request Mar 30, 2019
jsumners added 3 commits Mar 30, 2019
Copy link
Member

mcollina left a comment

Good work, I think you can go ahead and drop node 6 as well.

@jsumners

This comment has been minimized.

Copy link
Contributor Author

jsumners commented Mar 31, 2019

I intend to.

@jsumners

This comment has been minimized.

Copy link
Contributor Author

jsumners commented Mar 31, 2019

FYI: stack formatting is a nightmare.

jsumners added 6 commits Mar 31, 2019
Copy link
Member

mcollina left a comment

LGTM

jsumners added 11 commits Apr 6, 2019
jsumners and others added 7 commits Apr 9, 2019
* test coverage for filtering out with search

* obsolete check, ignored keys already filtered

* basic tests for prettifyErrorLog & prettifyObject
@jsumners

This comment has been minimized.

Copy link
Contributor Author

jsumners commented Apr 13, 2019

I think this is as done as I have time for. It would be nice to get a robust set of sample data to feed through it to at least generate a screenshot for the readme, but that can be a "good first issue" or I will come back to it later. We might as well wait for Node 12 to be released before closing this out since it is so close and this PR drops Node 6.

In the meantime, if anyone can think of how to cover https://coveralls.io/builds/22789235/source?filename=bin.js#L45 in the tests, a PR is welcome. The only other block Coveralls thinks is not covered is most definitely covered as I have stepped through it via the test suite.

It would also be fantastic if someone with more patience and time for formatting would come through and make the output even prettier. But it's good enough as-is.

jsumners added 5 commits Apr 21, 2019
@jsumners jsumners merged commit 310da6a into master Apr 28, 2019
3 checks passed
3 checks passed
continuous-integration/travis-ci/pr The Travis CI build passed
Details
continuous-integration/travis-ci/push The Travis CI build passed
Details
coverage/coveralls Coverage increased (+1.4%) to 99.415%
Details
@jsumners jsumners deleted the plain-ndjson branch Apr 28, 2019
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
4 participants
You can’t perform that action at this time.