Skip to content

Citations: parse json from HTTP response object instead of from disk. - #612

Closed
PietrH wants to merge 12 commits into
521-encoding-issuefrom
521-json-in-memory
Closed

Citations: parse json from HTTP response object instead of from disk.#612
PietrH wants to merge 12 commits into
521-encoding-issuefrom
521-json-in-memory

Conversation

@PietrH

@PietrH PietrH commented Aug 20, 2026

Copy link
Copy Markdown
Member

Reading the json from disk for every marineinfo request breaks the vcr http response caching in the tests for get_package() since the temp json files don't exist in the testing env.

This PR switches back to parsing these json response bodies directly from the response in memory.

@PietrH PietrH self-assigned this Aug 20, 2026
@PietrH
PietrH deployed to manual-approval August 20, 2026 08:01 — with GitHub Actions Active
@PietrH
PietrH requested a lite review from Copilot August 20, 2026 08: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.

Pull request overview

This PR updates the MarineInfo citation retrieval flow to parse dataset JSON directly from the httr2 HTTP response objects, and refreshes test snapshots to reflect the updated citation/tag fields.

Changes:

  • Parse MarineInfo dataset JSON via httr2::resp_body_json() instead of jsonlite::read_json() from disk.
  • Update citation snapshot output for additional projects (e.g., demer, dijle).
  • Update receivers snapshot output to include built_in_acoustic_tag_id for specific receivers.

Reviewed changes

Copilot reviewed 3 out of 5 changed files in this pull request and generated 1 comment.

File Description
R/cite_imis_dataset.R Switches JSON parsing to use the HTTP response object.
tests/testthat/_snaps/get_package/references.csv Updates expected citation snapshot output.
tests/testthat/_snaps/get_package/receivers.csv Updates expected receivers snapshot output (tag ID field).

💡 Add a code-review agent skill or configure MCP servers for context-aware, tailored reviews. Learn more in the docs.

Comment thread R/cite_imis_dataset.R
@codecov

codecov Bot commented Aug 20, 2026

Copy link
Copy Markdown

Codecov Report

✅ All modified and coverable lines are covered by tests.
⚠️ Please upload report for BASE (521-encoding-issue@78fc36c). Learn more about missing BASE report.

Additional details and impacted files
@@                  Coverage Diff                  @@
##             521-encoding-issue     #612   +/-   ##
=====================================================
  Coverage                      ?   95.07%           
=====================================================
  Files                         ?       34           
  Lines                         ?     1646           
  Branches                      ?        0           
=====================================================
  Hits                          ?     1565           
  Misses                        ?       81           
  Partials                      ?        0           

☔ View full report in Codecov by Harness.
📢 Have feedback on the report? Share it here.

🚀 New features to boost your workflow:
  • ❄️ Test Analytics: Detect flaky tests, report on failures, and find test suite problems.

@PietrH
PietrH deployed to manual-approval August 20, 2026 08:10 — with GitHub Actions Active
@PietrH

PietrH commented Aug 20, 2026

Copy link
Copy Markdown
Member Author

The R4.1 failure is due to a snapshot. A different bug than this PR is trying to fix.

…g packages. CRAN doesn't provide old binaries.
@PietrH
PietrH deployed to manual-approval August 20, 2026 09:28 — with GitHub Actions Active
… building packages. CRAN doesn't provide old binaries."

This reverts commit c751e95.
@PietrH
PietrH deployed to manual-approval August 20, 2026 09:59 — with GitHub Actions Active
@PietrH
PietrH deployed to manual-approval August 20, 2026 13:52 — with GitHub Actions Active
@PietrH
PietrH deployed to manual-approval August 20, 2026 14:58 — with GitHub Actions Active
@PietrH
PietrH deployed to manual-approval August 21, 2026 08:08 — with GitHub Actions Active

@peterdesmet peterdesmet left a comment

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

Did not review in detail but looks like your simplifying code, so I'm all for it.

Use NFC normalisation to normalize citations, avoiding encoding issues.

Was already reviewed in #612
@PietrH
PietrH deployed to manual-approval August 21, 2026 13:47 — with GitHub Actions Active
@PietrH PietrH closed this Aug 21, 2026
@PietrH
PietrH deleted the 521-json-in-memory branch August 21, 2026 13:48
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.

3 participants