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

Add singleLine config option #158

Merged
merged 2 commits into from
Mar 2, 2021

Conversation

LucasPickering
Copy link
Contributor

@LucasPickering LucasPickering commented Feb 22, 2021

This adds a singleLine config option. When enabled, all unrecognized non-error fields will be printed on the same line as the main log message, as a simple JSON string. Error messages are still printed like normal, on a new line.

The implementation for this is very rough. I wanted to get feedback on the design/behavior before I spent any time polishing the code. I also haven't updated any docs yet. The tests should provide all the examples/behavior definitions needed though.

This builds off of #125 and hopefully fixes #97.

Remaining work:

  • Clean up code/add comments
  • Add docs

@coveralls
Copy link

coveralls commented Feb 22, 2021

Pull Request Test Coverage Report for Build 590866056

  • 39 of 39 (100.0%) changed or added relevant lines in 2 files are covered.
  • No unchanged relevant lines lost coverage.
  • Overall coverage remained the same at 100.0%

Totals Coverage Status
Change from base Build 582319271: 0.0%
Covered Lines: 268
Relevant Lines: 268

💛 - Coveralls

@jsumners
Copy link
Member

Is this meant to include all of the work done in #125? If so, please see my comment in the PR for instructions on how to start your branch so that the original author is credited for their work.

@LucasPickering
Copy link
Contributor Author

Is this meant to include all of the work done in #125? If so, please see my comment in the PR for instructions on how to start your branch so that the original author is credited for their work.

Sorry about that. I just pushed to include their commit. I had to make some changes to the test to match my implementation but the structure of them is the same.

Copy link
Member

@mcollina mcollina left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

lgtm

lib/utils.js Show resolved Hide resolved
lib/utils.js Outdated Show resolved Hide resolved
lib/utils.js Show resolved Hide resolved
Readme.md Outdated Show resolved Hide resolved
bin.js Outdated Show resolved Hide resolved
Copy link
Member

@jsumners jsumners left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM

@LucasPickering
Copy link
Contributor Author

LGTM

Thanks for bearing with me on this. I rebased master + squashed, is there anything else I need to do before this can be merged?

@mcollina mcollina merged commit d724808 into pinojs:master Mar 2, 2021
@gr2m
Copy link
Contributor

gr2m commented Mar 2, 2021

Yay, thank you so much for implementing this ❤️

@LucasPickering LucasPickering deleted the issue-97-single-line branch March 2, 2021 18:19
LucasPickering referenced this pull request in MarkForged/pino-pretty Mar 5, 2021
A bug in #158 left a trailing space on lines that had no extra log fields with singleLine=true. This commit fixes that, and updates a test accordingly.
mcollina pushed a commit that referenced this pull request Mar 8, 2021
A bug in #158 left a trailing space on lines that had no extra log fields with singleLine=true. This commit fixes that, and updates a test accordingly.
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.

Add option for single-line logging of extra attributes
5 participants