Merged
Conversation
A2.1 - Remove debug statements:
- fg_pip.R: removed print('here') from fg_remove_duplicates(), #print('ZP:...')
A2.2 - Remove commented-out code blocks:
- compute_fgt_new.R: removed commented pov_from_DT2() body
- zzz.R: removed dead assign/memo_norm/detectCores commented blocks
- fg_pip.R: removed commented fg_standardize_cache_id() block
A2.3 - Remove unused functions:
- compute_fgt_new.R: removed DT_fgt_by_rl, lt_to_dt, map_lt_to_dt, map_fgt, pov_from_DT
- utils-pipdata.R: removed transform_input, get_rl_rows_single, get_rl_rows, get_dt_dist_stats, get_lt_attr
- utils.R: removed collapse_rows, convert_empty
- pip_grp_new.R: removed list_code_column_values
- create_lkups.R: removed coerce_chr_to_fct + 3 commented call sites
A2.4 - TO BE REMOVED blocks in pip_new_lineups.R left in place:
group_by param still flows through plumber endpoint; needs discussion before removal
A2.5+A2.6 - Convert TEMP markers to TODO:
- rg_pip.R, pip_new_lineups.R, utils.R: TEMPORARY FIX -> TODO with description
- create_lkups.R: all TEMP START/END markers converted to descriptive TODO comments
…sts (A1) A1.1 - tests/testdata/generate_snapshots.R: Run manually with PIPAPI_DATA_ROOT_FOLDER_LOCAL set to generate 8 .rds snapshot files covering: single country, all years, fill-gaps, all countries, multi-reporting-level, aggregation, multi-poverty-line, popshare. A1.2 - tests/testthat/test-snapshot-baseline.R: Regression tests using expect_equal(tolerance=1e-10) against saved snapshots. Tests skip cleanly when snapshots or data folder are unavailable. Run with: devtools::test(filter = 'snapshot')
…t test - Remove interactive rstudioapi/remotes lines from generate_snapshots.R - Add header comment explaining PROD version pin and why not to change it - Add rlang::abort() guard for missing env var - Add missing 9th test (snap_pip_wld_pov) to test-snapshot-baseline.R
…s (A2.4) - Remove both **** TO BE REMOVED **** blocks from pip_new_lineups() - Remove group_by param from function signature, roxygen @param, and match.arg - Remove group_by example from @examples block - /api/v1/pip endpoint: mark group_by as deprecated in doc, strip it from params before do.call so existing callers don't error - Aggregation is now exclusively via pip_agg() / /api/v1/pip-grp
utils.R (~900 lines, 30+ functions) split into 6 files by responsibility: - utils-lkup.R: lookup/filter helpers (subset_lkup, select_country, etc.) - utils-stats.R: stats enrichment (add_dist_stats, add_pg, add_spl, etc.) - utils-censor.R: censoring + estimate_type classifiers - utils-aux.R: thin wrappers over get_aux_table() for specific datasets - utils-query.R: API query controls + .check_group_by() - utils-misc.R: general helpers (is_empty, get_caller_names, etc.) Zero logic changes. Package loads cleanly after split.
…il (B2) Extract ~80 lines of identical post-processing logic from pip_new_lineups() and pip_old_lineups() into a new shared helper pip_lineups_format_output() in R/pip_lineups_postprocess.R. The only divergence between the two pathways is the dist-stats merge: - new pathway: add_dist_stats(df, lkup, fill_gaps) - old pathway: add_dist_stats_old(df, dist_stats = lkup[[dist_stats]]) This is handled via use_old_dist_stats = FALSE/TRUE.
…x snapshot test setup - pip.R: stop passing group_by= to pip_new_lineups() (new pathway dropped that parameter in A2.4); old pathway still receives it unchanged. - test-snapshot-baseline.R: wrap create_versioned_lkups() in tryCatch so a missing file skips rather than errors; pin to SNAP_VINTAGE so tests use the same data version as the saved snapshots.
…C1+C2); add unit tests for compute_fgt, pip_lineups_format_output, utils-stats, utils-pipdata (C3-C6); add roxygen @noRd/@Keywords internal to undocumented internals (C7)
…coutnry_code typo (P2); drop stale header comment, fix %||% in test, add .cg-docs to .Rbuildignore (P2/P3)
…total_pop, fgt_cumsum (Step 1b)
…ts, monotonicity (Step 1a)
…a helpers (Step 1c)
…g level) (Step 2b)
…_level, filter_lkup, lkup_filter (Step 2d)
…, sort_versions edge cases, create_return_cols (Step 2f)
…tors and get_additional_indicators_grp (Step 2h)
…gional aggs (Step 4)
P1 test-pip-unit.R: replace undefined lkups with est_lkups and add skip_if guard so CI never hard-errors on missing data. P2 test-pip_grp-unit.R, test-integ-regional-agg.R: move local_mocked_bindings() inside each test_that() block so the get_caller_names mock is properly scoped and cannot leak across test files. P3 test-add_agg_stats.R: replace informal 'This test is wrong' comment with a TODO comment. P3 test-censor_rows.R: guard top-level readRDS() calls with a skip() when fixture files are missing. P3 test-pip.R, test-pip_grp.R: replace empty RETIRED stubs with a single skip() test so testthat reports them cleanly. P3 test-compute_fgt.R: add explicit assertions for the documented collapse::setv() edge case (poverty_gap/severity = 0 for n=1) so any future regression is caught immediately.
|
Caution Review failedThe pull request is closed. ℹ️ Recent review info⚙️ Run configurationConfiguration used: defaults Review profile: CHILL Plan: Pro Run ID: 📒 Files selected for processing (52)
📝 WalkthroughWalkthroughMajor version release (1.5.0) reorganizing core utility functions across new modular files, refactoring FGT computation output structure, removing group_by parameter from pip function, adding comprehensive lookup validation, and expanding test coverage significantly. Large utils.R file split into specialized modules (utils-aux, utils-censor, utils-lkup, utils-misc, utils-query, utils-stats). Changes
Estimated code review effort🎯 5 (Critical) | ⏱️ ~120 minutes Poem
✨ Finishing Touches🧪 Generate unit tests (beta)
📝 Coding Plan
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 Tip CodeRabbit can suggest fixes for GitHub Check annotations.Configure the |
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
1 participant
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 by CodeRabbit
Release Notes
New Features
Bug Fixes
Deprecated
group_byparameter from the main/api/v1/pipendpoint; use/api/v1/pip-grpfor aggregation instead.Chores