Repository navigation
Fixes: more verbose control, validations passing, minor bug in check_status condition - #115
Conversation
📝 WalkthroughWalkthroughThis PR adds a Changes
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~50 minutes Poem
🚥 Pre-merge checks | ✅ 3✅ Passed checks (3 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches🧪 Generate unit tests (beta)
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 11
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (3)
R/utils.R (1)
459-473:⚠️ Potential issue | 🟠 MajorDefaulting
pip_aux_save()to verbose mode causes unintended behavior drift.At Line 461,
verbose = TRUEmakes every existing caller that omitsverbosebecome noisy, even when its public API defaults toverbose = FALSE. This silently weakens the new verbosity control.Proposed fix
pip_aux_save <- \(x, id, - verbose = TRUE, + verbose = FALSE, ...) {🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@R/utils.R` around lines 459 - 473, The function pip_aux_save currently sets verbose = TRUE which changes behavior for callers that omit verbose; change the default to FALSE (or derive it from a shared verbosity option) so callers keep existing non-noisy behavior, and ensure the value passed into pipload::pip_write still uses the pip_aux_save verbose parameter; update the pip_aux_save signature and any related usage (pip_aux_save, pipload::pip_write, get_from_auxenv("aux_alias")) accordingly so no callers are made noisy by default.R/aux_weo.R (1)
278-295:⚠️ Potential issue | 🟠 MajorThese raw-type checks now reject the package's own WEO input shape.
tests/testthat/test-weo.R:3-19buildsWEO Country Codeas character (c("111", "112")) andEstimates Start Afteras numeric. Lines 278-295 flip those expectations, so valid raw inputs now fail validation beforeaux_weo_clean()gets a chance to normalize them.Suggested fix
- validate_if(is.numeric(`WEO Country Code`), - description = "`WEO Country Code` should be numeric") |> + validate_if(is.character(`WEO Country Code`), + description = "`WEO Country Code` should be character") |> @@ - validate_if(is.character(`Estimates Start After`), - description = "`Estimates Start After` should be character") |> + validate_if(is.numeric(`Estimates Start After`), + description = "`Estimates Start After` should be numeric") |>🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@R/aux_weo.R` around lines 278 - 295, The type checks for WEO columns are too strict and reject the package's valid raw inputs; update the validate_if calls that reference `WEO Country Code` and `Estimates Start After` (and any similar columns) to accept the raw types used in tests by allowing either numeric or character as valid (e.g., change is.numeric(`WEO Country Code`) to a predicate that returns TRUE for numeric OR character, and change is.character(`Estimates Start After`) to allow numeric OR character), keeping the same descriptive messages and leaving other validate_if checks unchanged so aux_weo_clean() can handle normalization.R/aux_ppp.R (1)
11-16:⚠️ Potential issue | 🟠 MajorDon't move
ppp_defaultsbehind the new flag.Line 15 shifts
ppp_defaultsfrom the 5th to 6th positional argument. Callers using positional syntax likeaux_ppp(action, owner, tag, detail, FALSE)will now passFALSEtoverboseinstead ofppp_defaults, breaking backward compatibility and silently changing default PPP behavior.Additionally,
verboseis not actually propagated toaux_ppp_update()— the call at line 55 omits it, and line 204 hardcodesverbose = FALSEinpipload::load_aux_data(), rendering the new parameter non-functional.Suggested fix
aux_ppp <- function(action = c("update", "load"), owner = getOption("pipfun.ghowner"), tag = NULL, detail = getOption("pipaux.detail.raw"), - verbose = FALSE, - ppp_defaults = TRUE) { + ppp_defaults = TRUE, + verbose = FALSE) {🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@R/aux_ppp.R` around lines 11 - 16, The function signature change moved ppp_defaults from the 5th to the 6th positional argument and also never wires the new verbose flag through, breaking callers and making verbose non-functional; restore the original parameter order in aux_ppp so ppp_defaults remains the 5th positional argument (place verbose after ppp_defaults), ensure the verbose argument is forwarded when calling aux_ppp_update() from aux_ppp, and stop hardcoding verbose = FALSE in the pipload::load_aux_data() call (use the verbose variable instead) so the flag actually takes effect.
🧹 Nitpick comments (4)
dev/01_dependency_and_update_runner.R (1)
44-45: Use the samelog_namevariable for log retrieval to avoid ID drift.You now create and pass
log_name; use that same value when loading diagnostics instead of relying on a hardcoded ID.Proposed fix
- log_obj <- pipfun::log_load(id = "pipaux_update_log", alias = "aux_meta", overwrite = TRUE) + log_obj <- pipfun::log_load(id = log_name, alias = "aux_meta", overwrite = TRUE)Also applies to: 54-54
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@dev/01_dependency_and_update_runner.R` around lines 44 - 45, You create log_name <- init_aux_log(overwrite = TRUE) but later load diagnostics using a hardcoded ID; change the diagnostics/load calls to use the log_name variable instead of the hardcoded identifier so the created log and subsequent retrieval reference the same ID (apply the same replacement for the second occurrence around line 54 as well).R/aux_countries.R (1)
8-12: Consider movingverboseafter...for consistency with R best practices, if applied across similar functions.At line 11,
verbose = FALSEbefore...allows positional arguments to potentially bind toverboseinstead of being captured by.... While the current codebase calls use named arguments exclusively, this pattern appears intentionally consistent acrossaux_censoringandaux_cp. Movingverboseafter...would be safer and clearer, but should be coordinated across all similar aux functions to maintain consistency.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@R/aux_countries.R` around lines 8 - 12, The function aux_countries currently declares verbose before ... which can cause positional arguments to bind to verbose; move the verbose parameter to after ... (i.e., change the signature of aux_countries so ... comes before verbose) to ensure extra positional args are captured by ...; apply the same change consistently to the sibling functions aux_censoring and aux_cp so all aux_* signatures remain aligned and callers are not broken.R/identify_changes.R (1)
333-334: Addverbose = verboseto the load_aux_data() call at line 323 for consistent console output behavior.The
verboseparameter is passed when loading the previous version (lines 332-334) but not when loading the most recent version (line 323), creating inconsistent logging behavior withincompare_vintage_versions().♻️ Proposed patch
- new_df <- tryCatch({ - pipload::load_aux_data(measure = measure) + new_df <- tryCatch({ + pipload::load_aux_data(measure = measure, verbose = verbose) },🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@R/identify_changes.R` around lines 333 - 334, The call to load_aux_data() that loads the most recent version inside compare_vintage_versions() is missing the verbose argument, causing inconsistent logging; update that load_aux_data(...) invocation (the one that prepares the "most recent" data) to include verbose = verbose so it matches the call used for the previous version and preserves consistent console output behavior.R/aux_dictionary.R (1)
10-12: Remove the unused...parameter from this exported function.The
...parameter is accepted in the signature but never forwarded to internal function calls (pipfun::load_from_ghon line 29 andpipload::load_aux_dataon line 54) nor consumed elsewhere in the function body. This can silently swallow misspelled or unsupported arguments from callers, creating a poor API contract. Either remove it or explicitly validate and forward it.♻️ Proposed patch
aux_dictionary <- function(action = c("update", "load"), owner = getOption("pipfun.ghowner"), tag = NULL, - verbose = FALSE, - ...) { + verbose = FALSE) {🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@R/aux_dictionary.R` around lines 10 - 12, The exported function currently accepts an unused ... parameter alongside tag and verbose; remove ... from the function signature (and update any export docs/NAMESPACE if necessary) so callers cannot silently pass unsupported args, or alternatively explicitly validate/forward ... to the internal calls pipfun::load_from_gh and pipload::load_aux_data if you intend to accept extra arguments; choose one approach and update the function signature and any internal calls accordingly.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@dev/01_dependency_and_update_runner.R`:
- Line 26: The function exposes a log_save parameter but then overrides it
inside the body by hardcoding log_save = TRUE (around the internal invocation at
the current hardcoded assignment), so update the function to honor the external
parameter: remove or replace the hardcoded log_save = TRUE with the function
parameter log_save (pass the variable through to whichever internal call or save
routine uses it). Ensure any default remains the same by keeping the parameter
default at the signature and forwarding log_save to the internal call instead of
a literal TRUE.
In `@R/aux_gdm.R`:
- Around line 197-198: Before calling unique(inv, by = c("country_code",
"surveyid_year", "survey_acronym")), detect and error on inventory conflicts by
grouping inv by the key (country_code, surveyid_year, survey_acronym) and
checking whether more than one distinct survey_id exists per group; if any group
has >1 distinct survey_id, throw an informative error listing the offending
key(s) (or their counts) so the data issue is surfaced, otherwise proceed to
deduplicate (keep the unique rows) as currently done; reference the inv variable
and the survey_id / key columns when implementing this validation.
In `@R/aux_metadata.R`:
- Around line 9-13: aux_metadata() accepts a verbose argument but does not
forward it to aux_metadata_update(), so calling aux_metadata(action="update",
verbose=TRUE) still uses the default quiet behavior; update the aux_metadata
function to pass the verbose flag through when action == "update" by adding
verbose = verbose to the aux_metadata_update(...) call (and similarly ensure any
other branch that calls aux_metadata_update uses the forwarded verbose), so that
the pfw loading call respects the explicit verbose setting.
- Around line 187-190: The function signature for aux_metaregion currently
places verbose before tag which breaks backward compatibility for positional
calls; change the parameter order in the aux_metaregion definition so tag comes
before verbose (i.e., action, owner, tag = NULL, verbose = FALSE) and update any
internal references to these parameters accordingly so existing positional calls
like aux_metaregion("load", owner, "2026_04") bind correctly to tag.
In `@R/aux_missing_data.R`:
- Around line 9-13: The function signatures for aux_missing_data() and aux_sna()
place verbose before tag which breaks the aux_* positional-argument convention;
update both function declarations to list tag before verbose (e.g., action,
owner, tag = NULL, verbose = FALSE) so positional calls like
aux_missing_data("load", owner, "2025_DEV") bind tag correctly, ensure default
values remain the same, and verify any internal calls (e.g., load_aux_data(...,
verbose = verbose)) still reference the verbose parameter name unchanged.
In `@R/aux_pfw.R`:
- Around line 623-626: The uniqueness validation (validate_if(... is_uniq(...,
is_alt_welf))) is inconsistent with how records are persisted in
aux_pfw_update() using key_cols <- c("country_code", "surveyid_year",
"welfare_type"), which can allow duplicates at save time; fix by making the
uniqueness check and persistence keys consistent — either include is_alt_welf in
the persisted key_cols in aux_pfw_update() (so pfw is written with
c("country_code","surveyid_year","welfare_type","is_alt_welf")) or tighten the
validation by removing is_alt_welf from the is_uniq call so validate_if enforces
uniqueness on country_code, surveyid_year and welfare_type to match key_cols.
Ensure you update the is_uniq(...) invocation or key_cols definition
accordingly.
- Around line 275-276: The validator call to validate_cols(in_set(...), region,
...) mistakenly includes "SAR" which is inconsistent with the package's region
code "SAS" (and legacy handling of NAC/SAS); update the in_set list used by
validate_cols to use "SAS" (or include both "SAR" and "SAS" if you intentionally
want to accept both) so valid South Asia rows pass validation, and make the same
change for the other occurrence that mirrors this validator (the second
validate_cols call around the other region check).
- Around line 11-15: The verbose flag added to aux_pfw is not propagated to the
update path and the internal country_list loader; pass the verbose parameter
through to aux_pfw_update when action == "update" (i.e., call
aux_pfw_update(..., verbose = verbose)) and remove or replace the hardcoded
verbose = FALSE in the country_list load (call country_list(..., verbose =
verbose) or respect the parent's verbose value). Ensure any other internal calls
in aux_pfw that invoke aux_pfw_update or country_list also forward the verbose
argument so aux_pfw(action="update", verbose=TRUE) produces output.
In `@R/aux_pop.R`:
- Around line 9-13: The verbose argument is not being forwarded into the update
branch of aux_pop, so calls like aux_pop(action="update", verbose=TRUE) never
propagate; update the aux_pop implementation to pass the verbose parameter
through when calling aux_pop_update (i.e., call aux_pop_update(..., verbose =
verbose)), ensuring the update path receives the flag and that any downstream
loading of country_list or related behavior respects verbose; verify
aux_pop_update's signature accepts verbose and add it if missing.
In `@R/aux_ppp.R`:
- Around line 165-169: The verbose argument in aux_ppp_update() is declared but
never used for the "update" path; fix by threading verbosity through: have
aux_ppp pass its verbose value into aux_ppp_update(), and inside
aux_ppp_update() pass that verbose flag into pipload::load_aux_data(...)
(replace the hardcoded verbose = FALSE) so caller intent is honored; update the
aux_ppp call site and the pipload::load_aux_data call in aux_ppp_update() to use
the verbose parameter (or remove the parameter entirely if you prefer to force
silent behavior).
In `@R/aux_wdi.R`:
- Around line 9-13: The aux_wdi function signature currently places verbose
before detail and the man page (man/aux_wdi.Rd) omits verbose; reorder the
function arguments in aux_wdi so detail = getOption("pipaux.detail.raw") appears
before verbose = FALSE (i.e., action, owner, tag, detail, verbose) to avoid
breaking positional calls, add documentation for verbose in man/aux_wdi.Rd with
its description, then rebuild the Rd files (e.g., via devtools::document() or
roxygen2) to regenerate updated documentation.
---
Outside diff comments:
In `@R/aux_ppp.R`:
- Around line 11-16: The function signature change moved ppp_defaults from the
5th to the 6th positional argument and also never wires the new verbose flag
through, breaking callers and making verbose non-functional; restore the
original parameter order in aux_ppp so ppp_defaults remains the 5th positional
argument (place verbose after ppp_defaults), ensure the verbose argument is
forwarded when calling aux_ppp_update() from aux_ppp, and stop hardcoding
verbose = FALSE in the pipload::load_aux_data() call (use the verbose variable
instead) so the flag actually takes effect.
In `@R/aux_weo.R`:
- Around line 278-295: The type checks for WEO columns are too strict and reject
the package's valid raw inputs; update the validate_if calls that reference `WEO
Country Code` and `Estimates Start After` (and any similar columns) to accept
the raw types used in tests by allowing either numeric or character as valid
(e.g., change is.numeric(`WEO Country Code`) to a predicate that returns TRUE
for numeric OR character, and change is.character(`Estimates Start After`) to
allow numeric OR character), keeping the same descriptive messages and leaving
other validate_if checks unchanged so aux_weo_clean() can handle normalization.
In `@R/utils.R`:
- Around line 459-473: The function pip_aux_save currently sets verbose = TRUE
which changes behavior for callers that omit verbose; change the default to
FALSE (or derive it from a shared verbosity option) so callers keep existing
non-noisy behavior, and ensure the value passed into pipload::pip_write still
uses the pip_aux_save verbose parameter; update the pip_aux_save signature and
any related usage (pip_aux_save, pipload::pip_write,
get_from_auxenv("aux_alias")) accordingly so no callers are made noisy by
default.
---
Nitpick comments:
In `@dev/01_dependency_and_update_runner.R`:
- Around line 44-45: You create log_name <- init_aux_log(overwrite = TRUE) but
later load diagnostics using a hardcoded ID; change the diagnostics/load calls
to use the log_name variable instead of the hardcoded identifier so the created
log and subsequent retrieval reference the same ID (apply the same replacement
for the second occurrence around line 54 as well).
In `@R/aux_countries.R`:
- Around line 8-12: The function aux_countries currently declares verbose before
... which can cause positional arguments to bind to verbose; move the verbose
parameter to after ... (i.e., change the signature of aux_countries so ... comes
before verbose) to ensure extra positional args are captured by ...; apply the
same change consistently to the sibling functions aux_censoring and aux_cp so
all aux_* signatures remain aligned and callers are not broken.
In `@R/aux_dictionary.R`:
- Around line 10-12: The exported function currently accepts an unused ...
parameter alongside tag and verbose; remove ... from the function signature (and
update any export docs/NAMESPACE if necessary) so callers cannot silently pass
unsupported args, or alternatively explicitly validate/forward ... to the
internal calls pipfun::load_from_gh and pipload::load_aux_data if you intend to
accept extra arguments; choose one approach and update the function signature
and any internal calls accordingly.
In `@R/identify_changes.R`:
- Around line 333-334: The call to load_aux_data() that loads the most recent
version inside compare_vintage_versions() is missing the verbose argument,
causing inconsistent logging; update that load_aux_data(...) invocation (the one
that prepares the "most recent" data) to include verbose = verbose so it matches
the call used for the previous version and preserves consistent console output
behavior.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro
Run ID: 42befba6-7bee-46d9-be34-4245dcb0651a
📒 Files selected for processing (29)
R/aux_censoring.RR/aux_countries.RR/aux_country_list.RR/aux_cp.RR/aux_cpi.RR/aux_dictionary.RR/aux_gdm.RR/aux_gdp.RR/aux_income_groups.RR/aux_indicators.RR/aux_maddison.RR/aux_metadata.RR/aux_missing_data.RR/aux_nan.RR/aux_npl.RR/aux_pce.RR/aux_pfw.RR/aux_pl.RR/aux_pop.RR/aux_ppp.RR/aux_regions.RR/aux_sna.RR/aux_wdi.RR/aux_weo.RR/check_status.RR/identify_changes.RR/update_aux_data.RR/utils.Rdev/01_dependency_and_update_runner.R
| @@ -24,7 +24,7 @@ run_ordered_update_diagnostics <- function( | |||
| owner = getOption("pipfun.ghowner"), | |||
| tag = NULL, | |||
| log_save = TRUE, | |||
There was a problem hiding this comment.
log_save parameter is currently ignored by the function body.
Line 26 exposes log_save, but Line 53 hardcodes log_save = TRUE. This makes caller intent ineffective.
Proposed fix
- log_save = TRUE,
+ log_save = log_save,Also applies to: 53-53
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@dev/01_dependency_and_update_runner.R` at line 26, The function exposes a
log_save parameter but then overrides it inside the body by hardcoding log_save
= TRUE (around the internal invocation at the current hardcoded assignment), so
update the function to honor the external parameter: remove or replace the
hardcoded log_save = TRUE with the function parameter log_save (pass the
variable through to whichever internal call or save routine uses it). Ensure any
default remains the same by keeping the parameter default at the signature and
forwarding log_save to the internal call instead of a literal TRUE.
| # Keep only one survey_id per key to avoid fan-out on merge | ||
| inv <- unique(inv, by = c("country_code", "surveyid_year", "survey_acronym")) |
There was a problem hiding this comment.
🧩 Analysis chain
🏁 Script executed:
# First, let's look at the aux_gdm.R file structure and the specific lines in context
wc -l R/aux_gdm.R && echo "---" && head -1 R/aux_gdm.RRepository: PIP-Technical-Team/pipaux
Length of output: 99
🏁 Script executed:
# Get context around lines 197-198 (expand to see function scope and usage)
sed -n '180,220p' R/aux_gdm.RRepository: PIP-Technical-Team/pipaux
Length of output: 977
🏁 Script executed:
# Find the function definition and understand inv's structure/purpose
sed -n '1,60p' R/aux_gdm.RRepository: PIP-Technical-Team/pipaux
Length of output: 1812
🏁 Script executed:
# Check if there are validation steps after line 198
sed -n '195,210p' R/aux_gdm.RRepository: PIP-Technical-Team/pipaux
Length of output: 480
🏁 Script executed:
# Search for how inv is used after the unique() call
rg -A 5 'inv <- unique\(inv, by' R/aux_gdm.RRepository: PIP-Technical-Team/pipaux
Length of output: 253
🏁 Script executed:
# Search for tests related to aux_gdm
find . -name "*.R" -type f | xargs grep -l "aux_gdm" | head -10Repository: PIP-Technical-Team/pipaux
Length of output: 110
🏁 Script executed:
# Look for any validation logic around duplicates in the entire file
sed -n '190,250p' R/aux_gdm.RRepository: PIP-Technical-Team/pipaux
Length of output: 1484
🏁 Script executed:
# Check if there are any unique() calls or duplicate checks elsewhere in the file
rg -n "unique|duplicat" R/aux_gdm.RRepository: PIP-Technical-Team/pipaux
Length of output: 308
🏁 Script executed:
# Look at the full merge context to understand if multiple survey_id values would cause issues
sed -n '195,210p' R/aux_gdm.RRepository: PIP-Technical-Team/pipaux
Length of output: 480
🏁 Script executed:
# Check the test file for inv duplicates or scenarios
cat tests/testthat/test-gdm.RRepository: PIP-Technical-Team/pipaux
Length of output: 6130
🏁 Script executed:
# Look at the validation checks around lines 344 and 403
sed -n '340,350p' R/aux_gdm.R
sed -n '399,410p' R/aux_gdm.RRepository: PIP-Technical-Team/pipaux
Length of output: 1155
🏁 Script executed:
# Get full context of lines 340-350 and 399-410 to see what these validations are checking
sed -n '335,355p' R/aux_gdm.R && echo "---" && sed -n '395,415p' R/aux_gdm.RRepository: PIP-Technical-Team/pipaux
Length of output: 1781
🏁 Script executed:
# Search for any prior handling or comments about inventory conflicts
rg -B 5 -A 5 "duplicate|conflicting|multiple" R/aux_gdm.RRepository: PIP-Technical-Team/pipaux
Length of output: 1126
🏁 Script executed:
# Check if there's any error handling or validation specifically for the inventory merge
sed -n '185,215p' R/aux_gdm.RRepository: PIP-Technical-Team/pipaux
Length of output: 785
🏁 Script executed:
# Let me verify the data.table::unique() behavior claim by checking documentation or usage
# Search for data.table imports and check if there are any comments about unique() behavior
rg -n "library|require" R/aux_gdm.R | head -5Repository: PIP-Technical-Team/pipaux
Length of output: 51
🏁 Script executed:
# Check the entire aux_gdm_update function to understand the full flow
sed -n '68,250p' R/aux_gdm.RRepository: PIP-Technical-Team/pipaux
Length of output: 4639
Add validation to detect inventory conflicts instead of silently resolving them.
The unique(..., by = ...) call removes fan-out on the merge, but it also silently keeps the first survey_id for each (country_code, surveyid_year, survey_acronym) key. If the inventory has conflicting survey_id values for that key, this masks a data quality issue rather than surfacing it.
The safer approach is to detect conflicts upfront and error explicitly:
Suggested fix
- # Keep only one survey_id per key to avoid fan-out on merge
- inv <- unique(inv, by = c("country_code", "surveyid_year", "survey_acronym"))
+ dup_inv <- inv[
+ ,
+ .(n_survey_id = uniqueN(survey_id)),
+ by = .(country_code, surveyid_year, survey_acronym)
+ ][n_survey_id > 1]
+
+ if (nrow(dup_inv) > 0) {
+ cli::cli_abort(
+ "Inventory contains multiple survey_id values for the same GDM merge key."
+ )
+ }
+
+ inv <- unique(inv)📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| # Keep only one survey_id per key to avoid fan-out on merge | |
| inv <- unique(inv, by = c("country_code", "surveyid_year", "survey_acronym")) | |
| dup_inv <- inv[ | |
| , | |
| .(n_survey_id = uniqueN(survey_id)), | |
| by = .(country_code, surveyid_year, survey_acronym) | |
| ][n_survey_id > 1] | |
| if (nrow(dup_inv) > 0) { | |
| cli::cli_abort( | |
| "Inventory contains multiple survey_id values for the same GDM merge key." | |
| ) | |
| } | |
| inv <- unique(inv) |
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@R/aux_gdm.R` around lines 197 - 198, Before calling unique(inv, by =
c("country_code", "surveyid_year", "survey_acronym")), detect and error on
inventory conflicts by grouping inv by the key (country_code, surveyid_year,
survey_acronym) and checking whether more than one distinct survey_id exists per
group; if any group has >1 distinct survey_id, throw an informative error
listing the offending key(s) (or their counts) so the data issue is surfaced,
otherwise proceed to deduplicate (keep the unique rows) as currently done;
reference the inv variable and the survey_id / key columns when implementing
this validation.
| aux_metadata <- function(action = c("update", "load"), | ||
| owner = getOption("pipfun.ghowner"), | ||
| tag = NULL, | ||
| verbose = FALSE, | ||
| detail = getOption("pipaux.detail.raw")) { |
There was a problem hiding this comment.
aux_metadata() still drops verbose in "update" mode.
Lines 9-13 add the new flag, but Lines 27-34 never pass it to aux_metadata_update(). So aux_metadata(action = "update", verbose = TRUE) still loads pfw on Line 73 with the default quiet behavior.
🔧 Minimal fix
aux_metadata_update(
owner = owner,
branch = branch,
tag = tag,
+ verbose = verbose,
detail = detail
)Also applies to: 27-34
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@R/aux_metadata.R` around lines 9 - 13, aux_metadata() accepts a verbose
argument but does not forward it to aux_metadata_update(), so calling
aux_metadata(action="update", verbose=TRUE) still uses the default quiet
behavior; update the aux_metadata function to pass the verbose flag through when
action == "update" by adding verbose = verbose to the aux_metadata_update(...)
call (and similarly ensure any other branch that calls aux_metadata_update uses
the forwarded verbose), so that the pfw loading call respects the explicit
verbose setting.
| aux_metaregion <- function(action = c("update", "load"), | ||
| owner = getOption("pipfun.ghowner"), | ||
| verbose = FALSE, | ||
| tag = NULL |
There was a problem hiding this comment.
🧩 Analysis chain
🏁 Script executed:
find . -name "aux_metadata.R" -o -name "aux_metaregion.R"Repository: PIP-Technical-Team/pipaux
Length of output: 87
🏁 Script executed:
sed -n '180,200p' ./R/aux_metadata.RRepository: PIP-Technical-Team/pipaux
Length of output: 679
🏁 Script executed:
rg 'aux_metaregion\s*\(' -A 2 --type rRepository: PIP-Technical-Team/pipaux
Length of output: 51
🏁 Script executed:
rg 'aux_metaregion' --type r -C 3Repository: PIP-Technical-Team/pipaux
Length of output: 801
🏁 Script executed:
rg 'aux_metaregion\s*\(' -r '$0' --type-list | head -20Repository: PIP-Technical-Team/pipaux
Length of output: 458
🏁 Script executed:
# Search more broadly for function calls
rg 'aux_metaregion\(' --no-filenameRepository: PIP-Technical-Team/pipaux
Length of output: 84
🏁 Script executed:
rg 'aux_metaregion\(' -B 2 -A 5Repository: PIP-Technical-Team/pipaux
Length of output: 389
🏁 Script executed:
cat man/aux_metaregion.RdRepository: PIP-Technical-Team/pipaux
Length of output: 736
🏁 Script executed:
# Search for actual calls to aux_metaregion with arguments
rg 'aux_metaregion\s*\(\s*["\']' -A 3 -B 1Repository: PIP-Technical-Team/pipaux
Length of output: 137
🏁 Script executed:
# Also check for any test files
find . -name "*test*" -o -name "*spec*" | grep -i auxRepository: PIP-Technical-Team/pipaux
Length of output: 1161
🏁 Script executed:
rg 'aux_metaregion' ./pipaux/tests ./testsRepository: PIP-Technical-Team/pipaux
Length of output: 51
🏁 Script executed:
# Search entire repository for aux_metaregion calls
git ls-files | xargs grep -l 'aux_metaregion' 2>/dev/nullRepository: PIP-Technical-Team/pipaux
Length of output: 117
Reorder parameters to maintain backward compatibility for positional arguments.
The current signature has verbose as the third positional argument and tag as the fourth. This breaks the API contract for any external code using positional arguments. For example, aux_metaregion("load", owner, "2026_04") would now incorrectly bind "2026_04" to verbose instead of tag. The documentation file is currently out of sync, showing only the older action, owner, tag parameters.
Reorder the parameters to place tag before verbose to preserve backward compatibility:
Parameter reordering
aux_metaregion <- function(action = c("update", "load"),
owner = getOption("pipfun.ghowner"),
- verbose = FALSE,
- tag = NULL
+ tag = NULL,
+ verbose = FALSE
) {📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| aux_metaregion <- function(action = c("update", "load"), | |
| owner = getOption("pipfun.ghowner"), | |
| verbose = FALSE, | |
| tag = NULL | |
| aux_metaregion <- function(action = c("update", "load"), | |
| owner = getOption("pipfun.ghowner"), | |
| tag = NULL, | |
| verbose = FALSE |
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@R/aux_metadata.R` around lines 187 - 190, The function signature for
aux_metaregion currently places verbose before tag which breaks backward
compatibility for positional calls; change the parameter order in the
aux_metaregion definition so tag comes before verbose (i.e., action, owner, tag
= NULL, verbose = FALSE) and update any internal references to these parameters
accordingly so existing positional calls like aux_metaregion("load", owner,
"2026_04") bind correctly to tag.
| aux_missing_data <- function(action = c("update", "load"), | ||
| owner = getOption("pipfun.ghowner"), | ||
| verbose = FALSE, | ||
| tag = NULL | ||
| ) { |
There was a problem hiding this comment.
🧩 Analysis chain
🏁 Script executed:
# First, let's examine the file to see the current signature
cat -n R/aux_missing_data.R | head -30Repository: PIP-Technical-Team/pipaux
Length of output: 1202
🏁 Script executed:
# Search for other aux_* functions in the codebase
fd -e R | xargs grep -l "^aux_" | head -20Repository: PIP-Technical-Team/pipaux
Length of output: 377
🏁 Script executed:
# Check for NAMESPACE or roxygen exports
cat DESCRIPTION
cat -n NAMESPACE 2>/dev/null || echo "No NAMESPACE file"Repository: PIP-Technical-Team/pipaux
Length of output: 4727
🏁 Script executed:
# Check git history to see what changed in this file
cd R && git log --oneline -n 5 aux_missing_data.R 2>/dev/null || echo "Git not available or no history"Repository: PIP-Technical-Team/pipaux
Length of output: 86
🏁 Script executed:
# Check what the previous version of the file looked like
git diff HEAD~1 R/aux_missing_data.R 2>/dev/null | head -50 || echo "Cannot get git diff"Repository: PIP-Technical-Team/pipaux
Length of output: 51
🏁 Script executed:
# Let's check a few other aux_* functions to see their signatures
cat -n R/aux_cpi.R | head -20Repository: PIP-Technical-Team/pipaux
Length of output: 918
🏁 Script executed:
# Check aux_gdp
cat -n R/aux_gdp.R | head -20Repository: PIP-Technical-Team/pipaux
Length of output: 842
🏁 Script executed:
# Check aux_pop
cat -n R/aux_pop.R | head -20Repository: PIP-Technical-Team/pipaux
Length of output: 834
🏁 Script executed:
# Look for calls to these functions in the codebase to see if any use positional arguments
rg "aux_missing_data\s*\(" -A 2 --type r | head -40Repository: PIP-Technical-Team/pipaux
Length of output: 423
🏁 Script executed:
# Check for load_aux_data calls with verbose parameter
rg "load_aux_data\s*\(" -A 3 --type r | head -50Repository: PIP-Technical-Team/pipaux
Length of output: 2649
🏁 Script executed:
# Check a few more aux_* functions to see the pattern
for file in aux_censoring aux_countries aux_cp aux_dictionary aux_gdm aux_indicators; do
echo "=== $file ==="
cat -n R/${file}.R | grep -A 10 "^[[:space:]]*${file} <- function" | head -12
doneRepository: PIP-Technical-Team/pipaux
Length of output: 189
🏁 Script executed:
# Look for any positional calls - more comprehensive search
rg "aux_(missing_data|cpi|gdp|pop|ppp)\s*\(\s*['\"]" --type rRepository: PIP-Technical-Team/pipaux
Length of output: 308
🏁 Script executed:
# Check if there's a git stash or uncommitted changes
git status --short 2>/dev/null | grep aux_missing_data || echo "No uncommitted changes to aux_missing_data"Repository: PIP-Technical-Team/pipaux
Length of output: 111
🏁 Script executed:
# Search for any load_aux_data calls within the aux_* functions themselves
rg "load_aux_data" R/aux_missing_data.R -A 2 -B 2Repository: PIP-Technical-Team/pipaux
Length of output: 594
🏁 Script executed:
# Better search for function definitions - looking at the actual patterns
rg "^aux_\w+\s*<-\s*function" -A 6 --type r | head -100Repository: PIP-Technical-Team/pipaux
Length of output: 4358
🏁 Script executed:
# Also check what the git show or git log says about changes
git log --all --oneline 2>/dev/null | head -20Repository: PIP-Technical-Team/pipaux
Length of output: 822
🏁 Script executed:
# Try to see the actual diff more carefully
git diff --stat HEAD 2>/dev/null || echo "No git diff available"Repository: PIP-Technical-Team/pipaux
Length of output: 51
🏁 Script executed:
# Check aux_sna more carefully - it also has verbose before tag
cat -n R/aux_sna.R | head -20Repository: PIP-Technical-Team/pipaux
Length of output: 820
🏁 Script executed:
# Look at what load_aux_data signature is in pipload package
# First check if it's defined locally
rg "^load_aux_data\s*<-\s*function" -A 5 --type rRepository: PIP-Technical-Team/pipaux
Length of output: 51
🏁 Script executed:
# Search for the pipload package references to understand what verbose parameter it expects
rg "pipload::load_aux_data" -B 2 -A 3 --type r | head -60Repository: PIP-Technical-Team/pipaux
Length of output: 2703
🏁 Script executed:
# Check the full signatures of aux_missing_data and aux_sna side by side to confirm
echo "=== aux_missing_data ===" && cat -n R/aux_missing_data.R | sed -n '9,13p'
echo "=== aux_sna ===" && cat -n R/aux_sna.R | sed -n '8,11p'Repository: PIP-Technical-Team/pipaux
Length of output: 640
🏁 Script executed:
# Verify that load_aux_data accepts verbose parameter
rg "def load_aux_data|function load_aux_data" -A 5 2>/dev/null || echo "Not defined locally - from pipload package"Repository: PIP-Technical-Team/pipaux
Length of output: 111
Move verbose parameter after tag in aux_missing_data() and aux_sna() to match other aux_* functions and prevent positional argument mismatches.
The current signature places verbose before tag, breaking the established pattern in other aux_* functions (aux_cpi, aux_gdp, aux_pop, aux_pl, aux_pfw, aux_wdi, aux_weo). This creates a public API consistency issue: a positional call like aux_missing_data("load", owner, "2025_DEV") binds the tag value to the verbose parameter instead, silently selecting the wrong release and potentially passing non-logical values to downstream load_aux_data(..., verbose = verbose) calls. aux_sna has the same issue.
Suggested fix
aux_missing_data <- function(action = c("update", "load"),
owner = getOption("pipfun.ghowner"),
- verbose = FALSE,
- tag = NULL
+ tag = NULL,
+ verbose = FALSE
) {Apply the same change to aux_sna().
📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| aux_missing_data <- function(action = c("update", "load"), | |
| owner = getOption("pipfun.ghowner"), | |
| verbose = FALSE, | |
| tag = NULL | |
| ) { | |
| aux_missing_data <- function(action = c("update", "load"), | |
| owner = getOption("pipfun.ghowner"), | |
| tag = NULL, | |
| verbose = FALSE | |
| ) { |
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@R/aux_missing_data.R` around lines 9 - 13, The function signatures for
aux_missing_data() and aux_sna() place verbose before tag which breaks the aux_*
positional-argument convention; update both function declarations to list tag
before verbose (e.g., action, owner, tag = NULL, verbose = FALSE) so positional
calls like aux_missing_data("load", owner, "2025_DEV") bind tag correctly,
ensure default values remain the same, and verify any internal calls (e.g.,
load_aux_data(..., verbose = verbose)) still reference the verbose parameter
name unchanged.
| validate_cols(in_set(c("EAP", "ECA", "LAC", "MNA", "NAC", "SAR", "SSA")), | ||
| region, description = "`region` values within range") |> |
There was a problem hiding this comment.
SAR looks inconsistent with the package's existing region codes.
These validators now accept SAR, but the nearby region-code updates still use SAS for South Asia in R/aux_countries.R:105-112, and R/aux_country_list.R:204-212 still treats NAC/SAS as the legacy codes. Unless the upstream PFW source really renamed South Asia to SAR, this change will reject valid SAS rows during validation.
Also applies to: 467-468
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@R/aux_pfw.R` around lines 275 - 276, The validator call to
validate_cols(in_set(...), region, ...) mistakenly includes "SAR" which is
inconsistent with the package's region code "SAS" (and legacy handling of
NAC/SAS); update the in_set list used by validate_cols to use "SAS" (or include
both "SAR" and "SAS" if you intentionally want to accept both) so valid South
Asia rows pass validation, and make the same change for the other occurrence
that mirrors this validator (the second validate_cols call around the other
region check).
| validate_cols(not_na, country_code, year, welfare_type, | ||
| description = "no missing values in key variables") |> | ||
| validate_if(is_uniq(country_code, year, welfare_type), | ||
| validate_if(is_uniq(country_code, year, welfare_type, is_alt_welf), | ||
| description = "no duplicate records in key variables") |> |
There was a problem hiding this comment.
The relaxed uniqueness check no longer matches the saved key.
Line 625 now allows two rows that differ only by is_alt_welf, but aux_pfw_update() still persists pfw with key_cols <- c("country_code", "surveyid_year", "welfare_type") on Lines 202-212. That means validation can pass while save-time keys are still ambiguous.
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@R/aux_pfw.R` around lines 623 - 626, The uniqueness validation
(validate_if(... is_uniq(..., is_alt_welf))) is inconsistent with how records
are persisted in aux_pfw_update() using key_cols <- c("country_code",
"surveyid_year", "welfare_type"), which can allow duplicates at save time; fix
by making the uniqueness check and persistence keys consistent — either include
is_alt_welf in the persisted key_cols in aux_pfw_update() (so pfw is written
with c("country_code","surveyid_year","welfare_type","is_alt_welf")) or tighten
the validation by removing is_alt_welf from the is_uniq call so validate_if
enforces uniqueness on country_code, surveyid_year and welfare_type to match
key_cols. Ensure you update the is_uniq(...) invocation or key_cols definition
accordingly.
| aux_pop <- function(action = c("update", "load"), | ||
| owner = getOption("pipfun.ghowner"), | ||
| tag = NULL, | ||
| verbose = FALSE, | ||
| detail = getOption("pipaux.detail.raw")) { |
There was a problem hiding this comment.
Thread verbose into the update branch.
Lines 9-13 add verbose to aux_pop(), but Lines 27-32 still call aux_pop_update() without it. That means aux_pop(action = "update", verbose = TRUE) never reaches the country_list load on Line 156, so the new flag only works in "load" mode.
🔧 Minimal fix
if (action == "update") {
aux_pop_update(
owner = owner,
branch = branch,
tag = tag,
+ verbose = verbose,
detail = detail)Also applies to: 27-32
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@R/aux_pop.R` around lines 9 - 13, The verbose argument is not being forwarded
into the update branch of aux_pop, so calls like aux_pop(action="update",
verbose=TRUE) never propagate; update the aux_pop implementation to pass the
verbose parameter through when calling aux_pop_update (i.e., call
aux_pop_update(..., verbose = verbose)), ensuring the update path receives the
flag and that any downstream loading of country_list or related behavior
respects verbose; verify aux_pop_update's signature accepts verbose and add it
if missing.
| aux_ppp_update <- function(owner = getOption("pipfun.ghowner"), | ||
| branch = NULL, | ||
| tag = NULL, | ||
| verbose = FALSE, | ||
| detail = getOption("pipaux.detail.raw")) { |
There was a problem hiding this comment.
🧩 Analysis chain
🏁 Script executed:
# First, let's locate and inspect the aux_ppp.R file
find . -name "aux_ppp.R" -type fRepository: PIP-Technical-Team/pipaux
Length of output: 82
🏁 Script executed:
# Read lines around 165-169 to see the function signature
sed -n '160,175p' R/aux_ppp.RRepository: PIP-Technical-Team/pipaux
Length of output: 599
🏁 Script executed:
# Read lines around 204 to see what's on that line
sed -n '200,210p' R/aux_ppp.RRepository: PIP-Technical-Team/pipaux
Length of output: 395
🏁 Script executed:
# Search for the aux_ppp function definition and how it calls aux_ppp_update
rg "aux_ppp_update" R/aux_ppp.R -A 3 -B 1Repository: PIP-Technical-Team/pipaux
Length of output: 692
🏁 Script executed:
# Find the aux_ppp function definition
rg "^aux_ppp\s*<-\s*function" R/aux_ppp.R -A 20Repository: PIP-Technical-Team/pipaux
Length of output: 792
🏁 Script executed:
# Check the entire aux_ppp_update function to see if verbose is used
sed -n '165,250p' R/aux_ppp.RRepository: PIP-Technical-Team/pipaux
Length of output: 2431
🏁 Script executed:
# Search for any use of the verbose variable within aux_ppp_update
rg "verbose" R/aux_ppp.R | head -20Repository: PIP-Technical-Team/pipaux
Length of output: 288
The verbose parameter in aux_ppp_update() is unused and cannot be honored for the action = "update" path.
When aux_ppp() calls aux_ppp_update(), it does not pass the verbose argument. Additionally, line 204 hardcodes verbose = FALSE when calling pipload::load_aux_data(), ignoring any caller-provided verbosity intent. The parameter should either be removed or properly threaded through to control verbosity.
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@R/aux_ppp.R` around lines 165 - 169, The verbose argument in aux_ppp_update()
is declared but never used for the "update" path; fix by threading verbosity
through: have aux_ppp pass its verbose value into aux_ppp_update(), and inside
aux_ppp_update() pass that verbose flag into pipload::load_aux_data(...)
(replace the hardcoded verbose = FALSE) so caller intent is honored; update the
aux_ppp call site and the pipload::load_aux_data call in aux_ppp_update() to use
the verbose parameter (or remove the parameter entirely if you prefer to force
silent behavior).
| aux_wdi <- function(action = c("update", "load"), | ||
| owner = getOption("pipfun.ghowner"), | ||
| tag = NULL, | ||
| verbose = FALSE, | ||
| detail = getOption("pipaux.detail.raw")) { |
There was a problem hiding this comment.
🧩 Analysis chain
🏁 Script executed:
# First, let's examine the aux_wdi.R file to see the current parameter order
cat -n R/aux_wdi.R | head -20Repository: PIP-Technical-Team/pipaux
Length of output: 831
🏁 Script executed:
# Search for all calls to aux_wdi function in the codebase
rg "aux_wdi\s*\(" -A 3Repository: PIP-Technical-Team/pipaux
Length of output: 546
🏁 Script executed:
# Look at the test file more completely
cat -n tests/testthat/test-wdi.RRepository: PIP-Technical-Team/pipaux
Length of output: 2571
🏁 Script executed:
# Check the man file to see the documented signature
cat man/aux_wdi.RdRepository: PIP-Technical-Team/pipaux
Length of output: 787
🏁 Script executed:
# Search for any R code that might be calling aux_wdi with positional args
rg "aux_wdi\(" --type r -B 2 -A 5Repository: PIP-Technical-Team/pipaux
Length of output: 445
🏁 Script executed:
# Check git history to see if parameters were recently reordered
git log --oneline -20 -- R/aux_wdi.RRepository: PIP-Technical-Team/pipaux
Length of output: 86
Synchronize aux_wdi documentation and reorder parameters to prevent positional argument breakage.
The function has verbose before detail (lines 12-13), but the man file (man/aux_wdi.Rd) lists only action, owner, tag, and detail—the verbose parameter is missing from documentation. Additionally, placing verbose before detail risks breaking any positional calls where the 5th argument is intended for detail. Reorder the parameters to place detail before verbose, then regenerate documentation.
Proposed fix
aux_wdi <- function(action = c("update", "load"),
owner = getOption("pipfun.ghowner"),
tag = NULL,
- verbose = FALSE,
- detail = getOption("pipaux.detail.raw")) {
+ detail = getOption("pipaux.detail.raw"),
+ verbose = FALSE) {📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| aux_wdi <- function(action = c("update", "load"), | |
| owner = getOption("pipfun.ghowner"), | |
| tag = NULL, | |
| verbose = FALSE, | |
| detail = getOption("pipaux.detail.raw")) { | |
| aux_wdi <- function(action = c("update", "load"), | |
| owner = getOption("pipfun.ghowner"), | |
| tag = NULL, | |
| detail = getOption("pipaux.detail.raw"), | |
| verbose = FALSE) { |
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@R/aux_wdi.R` around lines 9 - 13, The aux_wdi function signature currently
places verbose before detail and the man page (man/aux_wdi.Rd) omits verbose;
reorder the function arguments in aux_wdi so detail =
getOption("pipaux.detail.raw") appears before verbose = FALSE (i.e., action,
owner, tag, detail, verbose) to avoid breaking positional calls, add
documentation for verbose in man/aux_wdi.Rd with its description, then rebuild
the Rd files (e.g., via devtools::document() or roxygen2) to regenerate updated
documentation.
Summary by CodeRabbit
New Features
verboseparameter to control output verbosity during operations (defaults to silent mode).Improvements