json parser: collapse the parser indirection into a single JsonParser implementation by Nikhil2206 · Pull Request #677 · eclipse-score/baselibs

Nikhil2206 · 2026-10-13T06:33:08Z

Closes #634

JsonParser::FromFile and FromBuffer are now defined in json_parser.cpp, next to the class they belong to.

The vaJson tree builder (VajsonParser) is no longer a public class in score::json. It is a file-local class (AnyTreeBuilder) in json_parser.cpp.
The vajson_parser library and its header and source, and the //score/json/internal/parser:parser alias, are removed. json_parser_impl links the vaJson reader directly.
The vaJson parser tests call JsonParser instead of the removed class.
The class diagram and the coverage scope list are updated accordingly.

Structural change only: the public API (IJsonParser, JsonParser, operator""_json) and the parsing behavior are unchanged. The parser logic is moved as is, including the coverage and Coverity annotations.

Tests: all //score/json/... tests pass, with the same test counts for the vaJson parser suites (27 and 16).

github-actions · 2026-10-13T06:48:23Z

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

Copilot

🟡 Changes recommended

Migrated tests retain obsolete requirement metadata, and the moved class members violate the documented member-comment convention.

4 open findings

What changed in this PR

Consolidates vaJson parsing into JsonParser, removing the redundant parser layer while preserving the public API.

Changes:

Moves tree-building and parsing into json_parser.cpp.
Removes VajsonParser, its target, and parser alias.
Updates tests, dependencies, coverage scope, and architecture documentation.

File	Description
`tools/coverage/BUILD`	Removes the obsolete coverage root.
`score/json/json_parser.cpp`	Adds the internal tree builder and parser implementation.
`score/json/internal/parser/vajson/vajson_parser.h`	Removes the redundant parser class.
`score/json/internal/parser/vajson/vajson_parser.cpp`	Removes the former implementation layer.
`score/json/internal/parser/vajson/vajson_parser_test.cpp`	Migrates parser tests to `JsonParser`.
`score/json/internal/parser/vajson/unit_test/BUILD`	Updates implementation-test dependencies.
`score/json/internal/parser/vajson/number_parser_test.cpp`	Migrates typed parsing tests.
`score/json/internal/parser/vajson/BUILD`	Removes the parser library and updates tests.
`score/json/internal/parser/BUILD`	Removes the parser alias.
`score/json/detailed_design/class_diagram.puml`	Documents the consolidated design.
`score/json/BUILD`	Links the vaJson implementation directly.

🧠 Review effort: Balanced

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

mihajlo-k · 2026-10-22T12:09:05Z

-)
-
 cc_test(
    name = "vajson_parser_test",


it feels like tests target was left behind while parser was moved

mihajlo-k · 2026-10-22T12:19:06Z

        ":parser_interface",
-        "@score_baselibs//score/json/internal/parser",
+        "@score_baselibs//score/json/internal/model",
+        "@score_baselibs//score/json/internal/parser/vajson/vajson_impl",


could this be moved to implementation_deps? generally all internal details should be handled that way.

Done. The internal model, vajson and result deps of json_parser_impl are now in implementation_deps, so they are no longer exposed to consumers.

mihajlo-k · 2026-10-22T12:20:20Z


 #include "score/json/json_parser.h"

+#include "score/json/internal/parser/vajson/vajson_impl/reader.h"


couldn't we flatten the directory structure a bit more?
vajson/vajson_impl seems redundant.

I have tried. vajson_impl is gone. reader/ and util/ now sit directly under vajson/, and the includes are updated to match\

…der internal JsonParser::FromFile and FromBuffer are now defined in json_parser.cpp next to the class. The vaJson tree builder is a file-local class of json_parser.cpp instead of the public VajsonParser, and the vajson_parser library and the parser alias are removed. The public API and behavior are unchanged.

Address review feedback on eclipse-score#677. The vaJson tree builder moved into `json_parser.cpp`, but its two test targets stayed in `internal/parser/vajson/`. They exercise `JsonParser` and use the shared test suites from `internal/parser/`, so they now live next to them, alongside `json_parser_test`. `internal/parser/vajson/BUILD` held only these two targets and is removed. Test sources and target names are unchanged.

Address review feedback on eclipse-score#677. With the vaJson tree builder gone from `internal/parser/vajson/`, the `vajson_impl` subdirectory only repeated the name of its parent. The vaJson reader sources now sit directly in `internal/parser/vajson/`, and the library target is named after its directory: `//score/json/internal/parser/vajson`. All changes are renames plus rewritten include paths and BUILD labels. No source logic changes.

Address review feedback on eclipse-score#677. `json_parser_impl` has no public headers of its own. `json_parser.h` comes from `:parser_interface`, which stays in `deps`. The model, vaJson reader and result targets are only included from `json_parser.cpp`, so they move to `implementation_deps` and no longer leak into the dependency graph of consumers of `:json`. No source changes.

Copilot

🔵 Needs a closer look

The previously reported requirement-link mismatch in the hexadecimal test remains an unresolved verification-traceability concern.

No open issues 🎉

4 resolved since last review

_{Review effort: Balanced}

# Conflicts: # tools/coverage/BUILD

github-project-automation Bot added this to BAS - Baselibs FT Oct 13, 2026

Nikhil2206 had a problem deploying to workflow-approval October 13, 2026 06:33 — with GitHub Actions Error

github-project-automation Bot moved this to In Progress in BAS - Baselibs FT Oct 13, 2026

Nikhil2206 had a problem deploying to workflow-approval October 13, 2026 06:33 — with GitHub Actions Error

github-actions Bot added comp-json Related to score/json component c++ C++ code bazel Bazel and Starlark build files labels Oct 13, 2026

github-advanced-security Bot found potential problems Oct 13, 2026

View reviewed changes

Nikhil2206 force-pushed the nk-json-634 branch from cb2ab16 to 0ee14aa Compare October 13, 2026 07:22

Nikhil2206 temporarily deployed to workflow-approval October 13, 2026 07:22 — with GitHub Actions Inactive

4og requested a balanced review from Copilot October 13, 2026 08:00

Copilot started reviewing on behalf of 4og October 13, 2026 08:00

View session

Copilot AI reviewed Oct 13, 2026

View reviewed changes

Comment thread score/json/internal/parser/number_parser_test.cpp Resolved

Comment thread score/json/internal/parser/vajson_parser_test.cpp Resolved

Comment thread score/json/internal/parser/vajson_parser_test.cpp Resolved

Comment thread score/json/json_parser.cpp Resolved

Nikhil2206 temporarily deployed to workflow-approval October 14, 2026 06:43 — with GitHub Actions Inactive

Nikhil2206 force-pushed the nk-json-634 branch from 1808d4e to e548cb7 Compare October 19, 2026 07:49

Nikhil2206 had a problem deploying to workflow-approval October 19, 2026 07:50 — with GitHub Actions Error

Nikhil2206 marked this pull request as ready for review October 21, 2026 05:52

Nikhil2206 requested review from 4og, antonkri, arkjedrz, fbaeuerle and pawelrutkaq as code owners October 21, 2026 05:52

Nikhil2206 requested a review from mihajlo-k October 21, 2026 05:54

anmittag assigned Nikhil2206 Oct 21, 2026

Nikhil2206 force-pushed the nk-json-634 branch from 7c5e47b to 025ed6a Compare October 22, 2026 07:54

Nikhil2206 temporarily deployed to workflow-approval October 22, 2026 07:54 — with GitHub Actions Inactive

mihajlo-k reviewed Oct 22, 2026

View reviewed changes

Nikhil2206 added 4 commits October 23, 2026 10:13

Nikhil2206 force-pushed the nk-json-634 branch from 025ed6a to 9e28d87 Compare October 23, 2026 10:15

Nikhil2206 temporarily deployed to workflow-approval October 23, 2026 10:15 — with GitHub Actions Inactive

Merge branch 'main' into nk-json-634

a162551

Copilot AI reviewed Oct 26, 2026

View reviewed changes

Merge remote-tracking branch 'upstream/main' into nk-json-634

eb5f5ca

# Conflicts: # tools/coverage/BUILD

mihajlo-k approved these changes Oct 28, 2026

View reviewed changes

fbaeuerle approved these changes Oct 28, 2026

View reviewed changes


		#include "score/json/json_parser.h"

		#include "score/json/internal/parser/vajson/vajson_impl/reader.h"

Conversation

Nikhil2206 commented Oct 13, 2026

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

github-actions Bot commented Oct 13, 2026

Uh oh!

Copilot AI left a comment

Choose a reason for hiding this comment

🟡 Changes recommended

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

mihajlo-k Oct 22, 2026

Choose a reason for hiding this comment

Uh oh!

Nikhil2206 Oct 26, 2026

Choose a reason for hiding this comment

Uh oh!

mihajlo-k Oct 22, 2026

Choose a reason for hiding this comment

Uh oh!

Nikhil2206 Oct 26, 2026

Choose a reason for hiding this comment

Uh oh!

mihajlo-k Oct 22, 2026

Choose a reason for hiding this comment

Uh oh!

Nikhil2206 Oct 26, 2026 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Copilot AI left a comment

Choose a reason for hiding this comment

🔵 Needs a closer look

Uh oh!

This branch was previously deployed

Reviewers

Uh oh!

Assignees

Labels

Projects

Milestone

Development

Uh oh!

5 participants

Nikhil2206 Oct 26, 2026 •

edited

Loading