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.

Sign up for GitHub

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

Jump to bottom

fix: Ensure CLI outputs JSON to stdout when directed to pipe #804

Merged
merged 2 commits into from
Sep 18, 2022
Merged

fix: Ensure CLI outputs JSON to stdout when directed to pipe #804

merged 2 commits into from
Sep 18, 2022

Conversation

orpheuslummis
Copy link
Contributor

@orpheuslummis orpheuslummis commented Sep 15, 2022 •
edited
Loading

Relevant issue(s)

Resolves #803

Description

Small fix: Only output JSON when doing so to a pipe, to be compatible with JSON parsers etc.

Tasks

  • I made sure the code is well commented, particularly hard-to-understand areas.
  • I made sure the repository-held documentation is changed accordingly.
  • I made sure the pull request title adheres to the conventional commit style (the subset used in the project can be found in tools/configs/chglog/config.yml).
  • I made sure to discuss its limitations such as threats to validity, vulnerability to mistake and misuse, robustness to invalidation of assumptions, resource requirements, ...

How has this been tested?

make:test

Specify the platform(s) on which this was tested:

  • MacOS

@orpheuslummis orpheuslummis added action/no-benchmark Skips the action that runs the benchmark. area/cli Related to the CLI binary labels Sep 15, 2022
@orpheuslummis orpheuslummis added this to the DefraDB v0.3.1 milestone Sep 15, 2022
@orpheuslummis orpheuslummis self-assigned this Sep 15, 2022
@codecov
Copy link

codecov bot commented Sep 15, 2022 •
edited
Loading

Codecov Report

Merging #804 (83d7128) into develop (eccd4aa) will increase coverage by 0.00%.
The diff coverage is 100.00%.

Impacted file tree graph

@@           Coverage Diff            @@
##           develop     #804   +/-   ##
========================================
  Coverage    59.67%   59.67%           
========================================
  Files          155      155           
  Lines        17283    17283           
========================================
+ Hits         10313    10314    +1     
+ Misses        6044     6043    -1     
  Partials       926      926
Impacted Files Coverage Δ
cli/root.go 48.23% <ø> (+0.56%) ⬆️
cli/schema.go 100.00% <ø> (ø)
cli/cli.go 20.93% <100.00%> (+0.61%) ⬆️

@orpheuslummis orpheuslummis changed the title fix: CLI is JSON-only when outputting to pipe fix: Ensure CLI outputs JSON only when directed to pipe Sep 15, 2022
@fredcarle
Copy link
Collaborator

I'm not sure I understand what this changes. the logger doesn't output JSON for these lines by default. Starting defra from develop or this branch doesn't change anything.

Maybe you could add a test to show that it does what you want it to?

@orpheuslummis orpheuslummis marked this pull request as draft September 16, 2022 01:46
@orpheuslummis
Copy link
Contributor Author

thanks, you're right. i'll do another implementation,
but I suggest without a test because of the 0.3.1 release deadline in less than 24h. its test will be included in the upcoming CLI test suite.

@orpheuslummis orpheuslummis marked this pull request as ready for review September 16, 2022 19:16
@orpheuslummis orpheuslummis changed the title fix: Ensure CLI outputs JSON only when directed to pipe fix: Ensure CLI outputs JSON to stdout when directed to pipe Sep 16, 2022
@orpheuslummis orpheuslummis requested review from fredcarle and shahzadlone and removed request for fredcarle September 16, 2022 19:16
fredcarle
fredcarle approved these changes Sep 17, 2022
View reviewed changes
Copy link
Collaborator

@fredcarle fredcarle left a comment

Choose a reason for hiding this comment

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

LGTM. Looking forward to the tests in 0.4

shahzadlone
shahzadlone approved these changes Sep 18, 2022
View reviewed changes
Copy link
Member

@shahzadlone shahzadlone left a comment

Choose a reason for hiding this comment

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

LGTM!

@orpheuslummis orpheuslummis force-pushed the orpheus/fix/json-when-pipe branch from 05e78d3 to 83d7128 Compare September 18, 2022 12:30
@orpheuslummis orpheuslummis merged commit 89f707c into develop Sep 18, 2022
@orpheuslummis orpheuslummis deleted the orpheus/fix/json-when-pipe branch September 18, 2022 12:44
shahzadlone pushed a commit to shahzadlone/defradb that referenced this pull request Feb 23, 2024
@orpheuslummis
fix: Ensure CLI outputs JSON to stdout when directed to pipe (sourcen…
1e7f830 
…etwork#804)
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@fredcarle fredcarle fredcarle approved these changes

@shahzadlone shahzadlone shahzadlone approved these changes

Assignees

@orpheuslummis orpheuslummis

Labels
action/no-benchmark Skips the action that runs the benchmark. area/cli Related to the CLI binary
Projects
None yet
Milestone
DefraDB v0.3.1
Development

Successfully merging this pull request may close these issues.

CLI: JSON-only when outputting to pipe
3 participants
@orpheuslummis @fredcarle @shahzadlone