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 cli option for log formatting (Closes #531) #784

Merged
merged 4 commits into from
Sep 10, 2023

Conversation

Baschtie
Copy link
Contributor

@Baschtie Baschtie commented Sep 9, 2023

What type of PR is this?
/kind feature

What this PR does / Why we need it:
this pr adds an option to chose between the following formatting option:

  • auto - default option, detects if running in an interactive terminal and will choose either "json" or "plain"
  • json - outputs logs in json
  • plain - uses default tracing_subscriber output
  • pretty - uses tracing_subscriber.pretty() output
How the options look

json

Bildschirmfoto 2023-09-09 um 19 10 04

plain

Bildschirmfoto 2023-09-09 um 19 09 47

pretty

Bildschirmfoto 2023-09-09 um 20 06 17

Which issue(s) this PR fixes:
Closes #531

Special notes for your reviewer:
Not sure about the naming, "detect" could also be "auto" or if "pretty" should be something like "debug". I am also not sure if the "ppretty" option is needed at all.

updated with renamed options

Copy link
Collaborator

@XAMPPRocky XAMPPRocky left a comment

Choose a reason for hiding this comment

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

Thank you for your PR! This looks good to me, however I would change detect to auto, pretty to plain, and ppretty to pretty. I think that's a much more clear naming convention for describing what each of those options do.

src/cli.rs Outdated Show resolved Hide resolved
src/cli.rs Show resolved Hide resolved
src/cli.rs Outdated Show resolved Hide resolved
@Baschtie
Copy link
Contributor Author

agree and updated 👍

@quilkin-bot
Copy link
Collaborator

Build Succeeded 🥳

Build Id: aadc107f-4281-4037-b657-afc1ff2ea21a

The following development images have been built, and will exist for the next 30 days:

To build this version:

git fetch [email protected]:googleforgames/quilkin.git pull/784/head:pr_784 && git checkout pr_784
cargo build

@XAMPPRocky XAMPPRocky merged commit d8e93b6 into googleforgames:main Sep 10, 2023
3 checks passed
@markmandel markmandel added kind/feature New feature or request area/operations Installation, updating, metrics etc labels Oct 13, 2023
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
area/operations Installation, updating, metrics etc kind/feature New feature or request size/s
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Output JSON or pretty logs based on terminal
4 participants