Skip to content

Replace httr with httr2 in spk_geoserv_dlv()#10

Merged
NewGraphEnvironment merged 1 commit intomainfrom
fix/httr2-migration
Feb 7, 2026
Merged

Replace httr with httr2 in spk_geoserv_dlv()#10
NewGraphEnvironment merged 1 commit intomainfrom
fix/httr2-migration

Conversation

@NewGraphEnvironment
Copy link
Copy Markdown
Owner

Closes #9

Migrates from deprecated httr to actively maintained httr2:

  • httr::GET()httr2::request() |> req_url_query() |> req_perform()
  • httr::write_disk()req_perform(path = ...)
  • httr::status_code()httr2::resp_status()
  • cat() messages → cli::cli_alert_success/cli_abort for consistency

All 44 tests passing.

NewGraphEnvironment pushed a commit that referenced this pull request Feb 6, 2026
- Add @importFrom httr2 request req_url_query req_error req_perform resp_status
- Add req_error(is_error = \(resp) FALSE) to prevent httr2 from auto-throwing
  on 4xx/5xx responses, allowing manual status check and error handling

Addresses Claude code review feedback on PR #10.

Co-Authored-By: Claude Sonnet 4.5 <noreply@anthropic.com>
@claude
Copy link
Copy Markdown

claude Bot commented Feb 6, 2026

Code review

No issues found. Checked for bugs and CLAUDE.md compliance.

Migrates from deprecated httr to actively maintained httr2:
- httr::GET() -> httr2::request() |> req_url_query() |> req_perform()
- httr::write_disk() -> req_perform(path = ...)
- httr::status_code() -> httr2::resp_status()
- cat() messages -> cli::cli_alert_success/cli_abort for consistency

Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com>
@claude
Copy link
Copy Markdown

claude Bot commented Feb 7, 2026

Review

Summary: Clean migration from deprecated httr to httr2. Code changes are correct and follow best practices.

Correctness: ✅

  • Proper httr2 API usage: request() |> req_url_query() |> req_perform()
  • Correct parameter unpacking with !!! in req_url_query()
  • req_error(is_error = \(resp) FALSE) correctly prevents automatic errors (allowing manual status check)
  • req_perform(path = file_out) replaces write_disk() correctly
  • Status code extraction with resp_status() is correct

Error Handling: ✅

  • File is written before status check (line 95), then status checked (line 97)
  • If status != 200, function aborts with cli_abort (line 113)
  • Error function correctly prevents automatic throwing so manual check can happen

Consistency: ✅

  • Messaging upgraded to cli functions (cli_alert_success, cli_abort)
  • All httr imports removed from NAMESPACE, httr2 imports added
  • DESCRIPTION dependency updated

Potential Issue: ⚠️ Minor - Non-blocking
Line 94: req_error(is_error = \(resp) FALSE) disables automatic error handling, but if the file write fails (e.g., disk full, permissions), the function will still write a file and only detect the error at status check. This is acceptable for WFS responses (which return 200 + error XML), but worth noting.

Recommendation: Approve and merge. All 44 tests reportedly passing per PR description.

@NewGraphEnvironment NewGraphEnvironment merged commit 534d196 into main Feb 7, 2026
3 checks passed
@NewGraphEnvironment NewGraphEnvironment deleted the fix/httr2-migration branch February 7, 2026 11:27
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.

Replace httr with httr2 in spk_geoserv_dlv()

1 participant