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

enhancement: Add pretty parameter for encode_json #370

Merged
merged 3 commits into from
Aug 7, 2023

Conversation

Hexta
Copy link
Contributor

@Hexta Hexta commented Aug 4, 2023

Add pretty parameter for encode_json function to produce pretty-printed JSON string.

closes #358

Add `pretty` parameter for `encode_json` function to produce pretty-printed JSON string.
@jszwedko jszwedko requested a review from pront August 4, 2023 14:02
Copy link
Contributor

@pront pront left a comment

Choose a reason for hiding this comment

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

@Hexta: Thank you for submitting this PR!

This LGTM except for the one comment I left.

@@ -1,8 +1,15 @@
use crate::compiler::prelude::*;

fn encode_json(value: Value) -> Resolved {
fn encode_json(value: Value, pretty: bool) -> Resolved {
Copy link
Contributor

Choose a reason for hiding this comment

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

It is a good practice to avoid bool parameters. There are many ways to refactor:

  • an options struct
  • enum instead of bool
  • extract implementation details and split encode_json into two functions

Copy link
Contributor

@pront pront left a comment

Choose a reason for hiding this comment

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

One more thing, please add a new entry in the CHANGELOG. Create a ##unreleased section if one doesn't exist.

@pront pront enabled auto-merge (squash) August 7, 2023 18:50
@pront pront merged commit 3093834 into vectordotdev:main Aug 7, 2023
8 checks passed
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 pretty print support for encode_json
2 participants