Skip to content

Add pretty printing for vajson serializer - #636

Merged
4og merged 9 commits into
eclipse-score:mainfrom
JuriSchroeder:vajson-serializer-pretty-print
Oct 2, 2026
Merged

4og merged 9 commits into
eclipse-score:mainfrom
JuriSchroeder:vajson-serializer-pretty-print

Conversation

@JuriSchroeder

Copy link
Copy Markdown
Contributor

No description provided.


/// \brief Returns the underlying output stream to write scalar values into
/// \return The output stream passed on construction.
auto Stream() const noexcept -> std::ostream&
@github-actions

Copy link
Copy Markdown
Contributor

Documentation preview for this pull request is available at:
pr-636: https://eclipse-score.github.io/baselibs/pr-636/

@4og
4og requested review from mihajlo-k and a balanced review from Copilot September 29, 2026 13:01

Copilot AI left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

Copilot review overview

🟡 Changes recommended

The newly added tests use legacy requirement metadata that must be replaced with current component-requirement links.

Review effort: Balanced
Findings: 2 Low severity

Open (2)
What changed in this PR

Adds optional pretty printing to the vaJSON serializer while preserving backend-specific behavior.

Changes:

  • Propagates a pretty_print option through JsonWriter and serializer backends.
  • Adds structural whitespace handling for nested JSON containers.
  • Adds compact and pretty-print serialization tests.
File Description
score/​json/​json_writer.h Exposes the formatting option.
score/​json/​json_writer.cpp Propagates formatting to backends.
score/​json/​json_writer_test.cpp Updates shared expectations and tests compact output.
score/​json/​internal/​writer/​writer_backend.h Extends backend interfaces.
score/​json/​internal/​writer/​vajson/​writer/​serializers/​util/​formatting_writer.h Implements whitespace and indentation.
score/​json/​internal/​writer/​vajson/​writer/​serializers/​structures/​serializer.h Uses the formatting writer type.
score/​json/​internal/​writer/​vajson/​writer/​serializers/​structures/​key_serializer.h Formats object members.
score/​json/​internal/​writer/​vajson/​writer/​serializers/​structures/​generic_value_serializer.h Formats values and arrays.
score/​json/​internal/​writer/​vajson/​writer/​serializers/​structures/​generic_value_serializer_impl.h Formats objects.
score/​json/​internal/​writer/​vajson/​vajson_serialize.h Extends vaJSON serialization APIs.
score/​json/​internal/​writer/​vajson/​vajson_serialize.cpp Configures formatting per serialization.
score/​json/​internal/​writer/​vajson/​vajson_serialize_test.cpp Covers pretty and compact output.
score/​json/​internal/​writer/​vajson/​vajson_backend.cpp Forwards formatting through vaJSON.
score/​json/​internal/​writer/​json_serialize/​json_serialize_backend.cpp Accepts but ignores formatting configuration.

💡 Configure MCP servers for context-aware, tailored reviews. Learn more in the docs.

Comment thread score/json/internal/writer/vajson/vajson_serialize_test.cpp Outdated
Comment thread score/json/json_writer_test.cpp Outdated

@mihajlo-k mihajlo-k left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

Nice implementation!
I have a couple of comments that could be addressed in the follow-up, thus non-blocking.

Comment on lines -59 to -60
#if defined(WRITER_VAJSON)
static constexpr auto expected = R"([1234,"string",{"key":"value"}])";

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

here the test coverage for compact formatting is lost. maybe now we can parametrize the tests here to cover both formatting.

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

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

Done

Comment thread score/json/json_writer.cpp Outdated
const score::filesystem::AtomicUpdateOwnershipFlags ownership) noexcept
: IJsonWriter{}, file_sync_mode_{file_sync_mode}, atomic_ownership_{ownership}
const score::filesystem::AtomicUpdateOwnershipFlags ownership,
const bool pretty_print) noexcept

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

If someone encounters a call like this:
JsonWriter writer{sync_mode, ownership, true};,
it might not be clear to them what true means, and they might need to look into the constructor itself to understand.

To make the API more understandable we could use scoped enum (e.g. PrettyPrint::kDisabled and PrettyPrint::kEnabled or Formatting::kCompact and Formatting::kPrettyPrint).

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

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

Done

@mihajlo-k

mihajlo-k commented Sep 29, 2026 •

Copy link
Copy Markdown
Contributor

It is worth noting that there is a slight deviation in formatting between json serializer and vajson serializer with pretty printing. Namely json serializer serializes the empty list/object in a way that it splits opening and closing brackets in two separate lines ([\n ] or {\n }), while vajson serializer with pretty printing writes them in a single line ([] or {}).

@4og

4og commented Sep 30, 2026

Copy link
Copy Markdown
Member

It is worth noting that there is a slight deviation in formatting between json serializer and vajson serializer with pretty printing. Namely json serializer serializes the empty list/object in a way that it splits opening and closing brackets in two separate lines ([\n ] or {\n }), while vajson serializer with pretty printing writes them in a single line ([] or {}).

The [] or {} in a single line make more sense, don't they?

JuriSchroeder and others added 2 commits October 1, 2026 12:43
Co-authored-by: Copilot Autofix powered by AI <175728472+Copilot@users.noreply.github.com>
Signed-off-by: JuriSchroeder <84773803+JuriSchroeder@users.noreply.github.com>
@JuriSchroeder
JuriSchroeder marked this pull request as ready for review October 1, 2026 11:32
@github-actions github-actions Bot added the bazel Bazel and Starlark build files label Oct 1, 2026
@4og
4og requested review from mihajlo-k and a balanced review from Copilot October 1, 2026 13:25

Copilot AI left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

Copilot review overview

🟡 Changes recommended

New tests use obsolete requirement metadata, and the new header violates documented header conventions.

Review effort: Balanced
Findings: 3 Low severity

Open (3)
Resolved since last review (2)

Comment thread score/json/internal/writer/vajson/vajson_serialize_test.cpp
@4og
4og enabled auto-merge October 2, 2026 07:32
@4og
4og added this pull request to the merge queue Oct 2, 2026
Merged via the queue into eclipse-score:main with commit bcea44c Oct 2, 2026
27 checks passed
@github-project-automation github-project-automation Bot moved this from On Hold to Done in BAS - Baselibs FT Oct 2, 2026

This branch was previously deployed

1 inactive deployment
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

bazel Bazel and Starlark build files c++ C++ code comp-json Related to score/json component

Projects

Status: Done

Development

Successfully merging this pull request may close these issues.

5 participants