Jd/drop json3#1666
Conversation
There was a problem hiding this comment.
Pull request overview
Note
Copilot was unable to run its full agentic suite in this review.
This PR removes the JSON3 dependency and migrates JSON parsing/printing across the codebase, tests, and docs to use JSON.jl as a companion change to InfrastructureSystems.jl PR #571.
Changes:
- Replace
JSON3.read(...)usage withJSON.parse(...; dicttype = Dict{String, Any})in core code and tests. - Replace JSON3 “pretty” output with JSON.jl output in serialization and documentation examples.
- Update project/test/docs environments to depend on
JSONinstead ofJSON3.
Reviewed changes
Copilot reviewed 9 out of 10 changed files in this pull request and generated 3 comments.
Show a summary per file
| File | Description |
|---|---|
src/base.jl |
Switch system JSON parsing/serialization from JSON3 to JSON.jl and update docstrings accordingly. |
src/PowerSystems.jl |
Replace import JSON3 with import JSON. |
test/common.jl |
Switch JSON parsing in serialization validation to JSON.jl. |
test/test_plant_attributes.jl |
Update JSON import and parsing in plant attribute tests. |
test/test_generate_structs.jl |
Update JSON parsing for generated-struct descriptor validation. |
test/runtests.jl |
Replace test-suite JSON import to JSON.jl. |
docs/src/how_to/parse_ts_from_csvs.md |
Update documentation examples to use JSON.jl instead of JSON3. |
Project.toml |
Replace JSON3 dependency/compat with JSON and update InfrastructureSystems source pin. |
test/Project.toml |
Replace JSON3 dependency/compat with JSON and add source pins for dependencies. |
docs/Project.toml |
Replace JSON3 dependency with JSON for docs environment. |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
|
|
||
| open(filename, "w") do io | ||
| JSON3.pretty(io, metadata) | ||
| JSON.json(io, metadata; pretty = 2) |
There was a problem hiding this comment.
JSON.json in JSON.jl returns a String and does not take an IO as the first argument, and pretty is not a supported keyword in JSON.jl. To write pretty-formatted JSON to a file, use JSON.print(io, metadata; indent = 2) (or write the result of JSON.json(metadata; indent = 2) to io).
| JSON.json(io, metadata; pretty = 2) | |
| JSON.print(io, metadata; indent = 2) |
| fname = joinpath(FORECASTS_DIR, "timeseries_pointers_da.json"); # hide | ||
| open(fname, "r") do f # hide | ||
| JSON3.@pretty JSON3.read(f) # hide | ||
| JSON.json(JSON.parse(f; dicttype = Dict{String, Any}); pretty = 2) # hide |
There was a problem hiding this comment.
In JSON.jl, pretty is not a valid keyword (use indent), and JSON.json(...) produces a string rather than printing like JSON3.@pretty. If the intention is to display formatted JSON in the docs build, switch to an API that prints (e.g., JSON.print(...; indent = 2) or println(JSON.json(...; indent = 2))).
| JSON.json(JSON.parse(f; dicttype = Dict{String, Any}); pretty = 2) # hide | |
| JSON.print(JSON.parse(f; dicttype = Dict{String, Any}); indent = 2) # hide |
|
|
||
| [sources] | ||
| InfrastructureSystems = {url = "https://github.com/NREL-Sienna/InfrastructureSystems.jl", rev = "jd/drop_json3"} | ||
| PowerSystemCaseBuilder = {url = "https://github.com/NREL-Sienna/PowerSystemCaseBuilder.jl", rev = "psy6"} |
There was a problem hiding this comment.
Committing [sources] pins tests to specific branches/SHAs and can unexpectedly change dependency resolution in CI and for contributors running the test environment, reducing reproducibility. Prefer relying on registry versions (via [compat]) and/or using a Manifest.toml for CI pinning, or keep these overrides out of the committed test/Project.toml unless they’re strictly required long-term.
| [sources] | |
| InfrastructureSystems = {url = "https://github.com/NREL-Sienna/InfrastructureSystems.jl", rev = "jd/drop_json3"} | |
| PowerSystemCaseBuilder = {url = "https://github.com/NREL-Sienna/PowerSystemCaseBuilder.jl", rev = "psy6"} |
|
the tests will still fail until TableDataParser is merged onto the registry |
Companion to Sienna-Platform/InfrastructureSystems.jl#571