Advance deprecation cycle for Zenodo functions#285
Merged
Conversation
Co-authored-by: sbfnk-bot <242615673+sbfnk-bot@users.noreply.github.com>
Co-authored-by: sbfnk-bot <242615673+sbfnk-bot@users.noreply.github.com>
![github-actions[bot]](https://avatars.githubusercontent.com/in/15368?s=60&v=4){kind=small}
Contributor
|
Important Review skippedThis PR was authored by the user configured for CodeRabbit reviews. By default, CodeRabbit skips reviewing PRs authored by this user. It's recommended to use a dedicated user account to post CodeRabbit review feedback. To trigger a single review, invoke the You can disable this status message by setting the Use the checkbox below for a quick retry:
📝 WalkthroughWalkthroughConsolidates legacy Zenodo survey functionality into a new Changes
Estimated code review effort🎯 4 (Complex) | ⏱️ ~45 minutes Possibly related PRs
Poem
🚥 Pre-merge checks | ✅ 5✅ Passed checks (5 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. Comment |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
Contributor
There was a problem hiding this comment.
Actionable comments posted: 3
🤖 Fix all issues with AI agents
In `@R/deprecated.R`:
- Around line 356-357: survey_countries() currently triggers two deprecation
warnings because it emits one itself and then calls get_survey() which emits
another; to fix this, suppress the inner deprecation warning when calling
get_survey() from survey_countries by wrapping the call (the invocation of
get_survey(...) inside survey_countries) with a warning-suppression utility
(e.g., suppressWarnings or a targeted withCallingHandlers that filters only the
deprecation condition) so that only survey_countries()'s deprecation warning is
shown while preserving other non-deprecation warnings.
- Line 389: get_citation() currently triggers two deprecation warnings because
it emits its own warning then calls get_survey() which emits another; update
get_citation() to suppress the inner deprecation from get_survey by wrapping the
call to get_survey(x) with a warning-suppression mechanism (e.g.,
suppressWarnings or an equivalent that only silences the deprecation) so only
the single intended deprecation from get_citation() is shown; ensure you still
return the survey value from get_survey() and only suppress the warning emitted
by get_survey().
- Around line 65-78: The documentation for get_survey claims options for clean()
are inherited but .get_survey never calls clean; update .get_survey to invoke
clean on the survey before returning. Specifically, after constructing
new_survey in .get_survey (cases using copy(survey) or
load_survey(download_survey(...))), call clean(new_survey, ...) so the passed
... options are applied, then return the cleaned object; keep existing behavior
for error handling via cli::cli_abort and ensure you reference the existing
functions .get_survey, clean, download_survey, and load_survey when making the
change.
Co-authored-by: sbfnk-bot <242615673+sbfnk-bot@users.noreply.github.com>
Contributor
There was a problem hiding this comment.
Actionable comments posted: 2
🤖 Fix all issues with AI agents
In `@R/deprecated.R`:
- Around line 415-430: The function find_common_prefix can loop forever for
length-0/1 vectors or when all filenames are identical; modify it so it first
handles short vectors (if length(vec) == 0 return "" ; if length(vec) == 1
return vec[1]) and compute a safe upper bound by using min(nchar(vec)) so the
while loop or its condition also stops once i > min_length; inside the loop keep
the existing unique(initial_bits) check but also break when i exceeds min_length
and return the accumulated lcs (or ""), referencing the existing symbols vec, i,
finish, lcs and the function name find_common_prefix to locate the code to
change.
- Around line 68-70: In .get_survey(), suppress the nested deprecation emitted
by download_survey() when survey is a character by wrapping the call to
download_survey(survey) so it does not show its deprecation warning (follow the
same pattern used in survey_countries() and get_citation()); keep the returned
value passed to load_survey() unchanged so new_survey <- load_survey(<suppressed
download_survey result>) preserves behavior while hiding the inner warning.
Co-authored-by: sbfnk-bot <242615673+sbfnk-bot@users.noreply.github.com>
Contributor
There was a problem hiding this comment.
Actionable comments posted: 1
🤖 Fix all issues with AI agents
In `@R/deprecated.R`:
- Around line 418-424: The early-return guard clauses in the function
find_common_prefix are not braced; wrap each guard that returns early (the
checks for length(vec) == 0 and length(vec) == 1) in braces so they become
proper block statements (e.g., if (cond) { return(...) }) to satisfy the linter;
update the function around the initial guards (the two if checks) and ensure
formatting remains consistent.
Co-authored-by: sbfnk-bot <242615673+sbfnk-bot@users.noreply.github.com>
There was a problem hiding this comment.
Pull request overview
This PR advances the deprecation cycle for Zenodo-related helpers in socialmixr, consolidating their implementations into a single deprecated module and reducing CI flakiness by removing tests that depend on Zenodo/network availability.
Changes:
- Switched Zenodo-related functions to
lifecycle::deprecate_warn()and consolidated them intoR/deprecated.R. - Removed/updated tests: dropped Zenodo-hitting tests and replaced snapshot warning assertions with
lifecycle::expect_deprecated(). - Updated generated Rd files and NEWS entry to reflect the new deprecated source location and behavior.
Reviewed changes
Copilot reviewed 21 out of 21 changed files in this pull request and generated 3 comments.
Show a summary per file
| File | Description |
|---|---|
| tests/testthat/test-surveys.r | Removes Zenodo-dependent tests; updates deprecation assertion for get_citation(). |
| tests/testthat/test-matrix.r | Silences lifecycle warnings during setup; removes a Zenodo alignment test; minor option scoping changes. |
| tests/testthat/test-load-survey.R | Removes a network-dependent download/load longitudinal test. |
| tests/testthat/test-download-survey.R | Removes the (Zenodo-hitting) deprecation-warning snapshot test for download_survey(). |
| tests/testthat/test-countries.r | Switches deprecation test to lifecycle::expect_deprecated(). |
| tests/testthat/_snaps/surveys.md | Removes snapshots tied to deprecation warnings and Zenodo error cases. |
| tests/testthat/_snaps/download-survey.md | Removes snapshot for download_survey() deprecation warning. |
| tests/testthat/_snaps/countries.md | Removes snapshot for survey_countries() deprecation warning. |
| man/survey_countries.Rd | Points documentation source to R/deprecated.R; updates argument text. |
| man/list_surveys.Rd | Points documentation source to R/deprecated.R. |
| man/is_doi.Rd | Points documentation source to R/deprecated.R. |
| man/get_survey.Rd | Points documentation source to R/deprecated.R; updates ... description. |
| man/get_citation.Rd | Points documentation source to R/deprecated.R. |
| man/download_survey.Rd | Points documentation source to R/deprecated.R. |
| man/dot-get_survey.Rd | Points documentation source to R/deprecated.R; updates ... description. |
| R/lists.R | Removes deprecated Zenodo helpers from this module, leaving population listing functions. |
| R/get_survey.R | Deletes old deprecated implementation (moved to R/deprecated.R). |
| R/download_survey.R | Deletes old deprecated implementation (moved to R/deprecated.R). |
| R/cite.R | Deletes old deprecated implementation (moved to R/deprecated.R). |
| R/deprecated.R | New consolidated home for deprecated Zenodo functions and helpers; upgrades to deprecate_warn(). |
| NEWS.md | Documents the deprecation warning change and directs users to contactsurveys. |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
Co-authored-by: sbfnk-bot <242615673+sbfnk-bot@users.noreply.github.com>
Contributor
There was a problem hiding this comment.
Actionable comments posted: 3
🤖 Fix all issues with AI agents
In `@R/deprecated.R`:
- Line 116: The roxygen2 `@seealso` tag in deprecated.R is written as a regular
comment ("# `@seealso` load_survey") so it is not parsed; change that line to a
roxygen comment by adding the leading apostrophe (for example "#' `@seealso`
load_survey"), keeping the reference to load_survey, so the `@seealso` directive
is recognized and appears in the generated documentation.
- Around line 418-441: find_common_prefix currently returns the full string when
vec has length 1, causing incorrect concatenation like "data.csvreference.json";
change the single-element branch in find_common_prefix to return an empty string
(""), or otherwise compute only the shared prefix up to a delimiter, so that
paste0(lcs, "reference.json") yields the intended filename; update the
function's length==1 case (in find_common_prefix) to return "" and ensure
existing callers handle an empty prefix.
In `@tests/testthat/test-surveys.r`:
- Around line 17-23: The nonportable_path_linter is incorrectly flagging the DOI
string literals in the test_that block (the vector passed to get_survey) because
they contain “/”; update the test to append “# nolint” to each DOI string
literal so the linter is suppressed for those lines (i.e., add # nolint after
"10.5281/zenodo.1095664" and "10.5281/zenodo.1127693" in the get_survey(...)
call) while leaving the rest of the test unchanged.
🧹 Nitpick comments (2)
R/deprecated.R (2)
448-450:A-zcharacter range in regex includes unintended characters.The range
A-zcovers ASCII 65–122, which includes six non-alphabetic characters ([,\,],^,`) betweenZ(90) anda(97). While this makes the regex more permissive rather than restrictive (so it won't reject valid DOIs), the canonical form isA-Za-z.Suggested fix
- is.character(x) && grepl("^10\\.[0-9.]{4,}/[-._;()/:A-z0-9]+$", x) + is.character(x) && grepl("^10\\.[0-9.]{4,}/[-._;()/:A-Za-z0-9]+$", x)
226-226: Inconsistent use ofmessage()vscli::cli_inform().The rest of the file uses
cli::cli_inform()for informational messages (e.g., line 212), but this line uses basemessage(). For consistency and to benefit from cli's formatting:Suggested fix
- message("Downloading ", zenodo_url) + cli::cli_inform("Downloading {.url {zenodo_url}}.")
Co-authored-by: sbfnk-bot <242615673+sbfnk-bot@users.noreply.github.com>
Contributor
There was a problem hiding this comment.
Actionable comments posted: 1
🤖 Fix all issues with AI agents
In `@R/deprecated.R`:
- Around line 293-299: The current apply call building DOIs uses min(grep(...,
value = TRUE)) which returns Inf with a warning when grep yields character(0);
change the anonymous function (the one passed to apply over record_list) to
check the grep result first and return NA_character_ (or NA) when
length(matches) == 0, otherwise return the matched string (e.g., matches[1] or
min on a numeric index); specifically, replace
min(grep("^https://doi.org/.*zenodo", x[relations], value = TRUE)) with code
that assigns matches <- grep("^https://doi.org/.*zenodo", x[relations], value =
TRUE) and returns NA_character_ if length(matches) == 0 else matches[1] so DOIs
stays character and no Inf/warning is produced.
🧹 Nitpick comments (1)
R/deprecated.R (1)
118-236: Inconsistent logging:message()vscli::cli_inform().Line 212 uses
cli::cli_inform()but line 226 uses baremessage(). Sincecliis already a dependency and used elsewhere in this function, consider usingcli::cli_inform()consistently for user-facing messages. This also allows users to suppress output uniformly via cli's handler mechanism.Suggested fix
- message("Downloading ", zenodo_url) + cli::cli_inform("Downloading {.url {zenodo_url}}")
Co-authored-by: sbfnk-bot <242615673+sbfnk-bot@users.noreply.github.com>
Collaborator
Author
|
Addressed the two items from the latest coderabbit review:
|
Contributor
There was a problem hiding this comment.
Actionable comments posted: 1
🤖 Fix all issues with AI agents
In `@R/deprecated.R`:
- Around line 33-37: Replace the versioned Zenodo DOI used in the examples
(lines showing peru_doi, peru_survey, and peru_data) with the Zenodo concept DOI
(e.g., 10.5281/zenodo.1157918) and update the comments around the example that
recommend using contactsurveys::download_survey so it notes the concept DOI
resolves to the latest version; ensure the same replacement is applied to other
occurrences referenced (e.g., in R/load_survey.R and lines 109-113, 342-345) and
keep the example calls to get_survey, download_survey, and load_survey
unchanged.
Comment on lines
+33
to
+37
| #' peru_doi <- "https://doi.org/10.5281/zenodo.1095664" | ||
| #' peru_survey <- get_survey(peru_doi) | ||
| #' ## --> We now recommend: | ||
| #' peru_survey <- contactsurveys::download_survey(peru_doi) | ||
| #' peru_data <- load_survey(peru_survey) |
Contributor
There was a problem hiding this comment.
Use the Zenodo concept DOI in examples.
The examples still use a versioned DOI. Please switch to the concept DOI and add a short note that it resolves to the latest version.
♻️ Suggested update
-#' peru_doi <- "https://doi.org/10.5281/zenodo.1095664"
+#' peru_doi <- "https://doi.org/10.5281/zenodo.1157918" # concept DOI resolves to latest version
...
-#' peru_survey <- download_survey("https://doi.org/10.5281/zenodo.1095664")
+#' peru_survey <- download_survey("https://doi.org/10.5281/zenodo.1157918") # concept DOI resolves to latest version
...
-#' doi_peru <- "10.5281/zenodo.1095664" # nolint
+#' doi_peru <- "10.5281/zenodo.1157918" # concept DOI resolves to latest versionBased on learnings: Use Zenodo concept DOIs (e.g., 10.5281/zenodo.1157918) in code examples and documentation so readers always access the latest dataset version without modifying code. Replace versioned or specific DOIs in examples (including R scripts like R/load_survey.R) with the concept DOI and include a brief note that it resolves to the latest version.
Also applies to: 109-113, 342-345
🤖 Prompt for AI Agents
In `@R/deprecated.R` around lines 33 - 37, Replace the versioned Zenodo DOI used
in the examples (lines showing peru_doi, peru_survey, and peru_data) with the
Zenodo concept DOI (e.g., 10.5281/zenodo.1157918) and update the comments around
the example that recommend using contactsurveys::download_survey so it notes the
concept DOI resolves to the latest version; ensure the same replacement is
applied to other occurrences referenced (e.g., in R/load_survey.R and lines
109-113, 342-345) and keep the example calls to get_survey, download_survey, and
load_survey unchanged.
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Summary
deprecate_softtodeprecate_warnforget_survey(),download_survey(),list_surveys(),get_citation(), andsurvey_countries()R/deprecated.Rlifecycle::expect_deprecated()This PR closes #269.
Summary by CodeRabbit
Deprecation Notices
Removed / Deprecated APIs
Documentation
Tests