Repository navigation
fix: systematic code review corrections (tests, download, phenodata, conventions) - #9
Merged
Merged
Conversation
- pass the sep argument through to parse_key_value_elements(); it only affected column detection before, so custom separators were parsed with ':' regardless - fix accidental logical-index recycling in characteristics column selection, which silently skipped valid non-contiguous columns - str_match() now returns an all-NA row with one column per capture group when nothing matches (stringr semantics) instead of collapsing to a 1-column matrix that crashes downstream [, 2L] indexing - drop five unused str_* helpers and fix the 'stingr' typo
Add test files for log_trans, parse_sample_data, geo_gtype/geo_url, geo_search, geo_meta, and the GEOSoft record accessors. Network-dependent tests reuse the package's skip-on-download-failure convention; geo_search gets a dedicated network-aware skip since it goes through rentrez/curl rather than the download layer. Also fix the download skip helper for cases where the destination exists.
- download_inform() regressed to requiring status 200 for every transfer, but plain-FTP completes with 226 (206/416 for ranged requests), so ftp_over_https = FALSE downloads were deleted and reported as failures; restore the documented multi-status check - fix undefined variable 'downloaded' in the annotation-failure fallback of parse_gse_matrix() (guaranteed crash when an annotation download fails) and stop embedding cli markup inside paste() - align geo_matrix_impl() dead defaults with the wrapper (pdata_from_soft, ftp_over_https) - abort with a clear message when sample metadata has no platform_id instead of cascading into a confusing rust error - build the default User-Agent header directly instead of reaching into curl internals
- validate the query argument of geo_search() like its siblings and merge the two entrez_search() calls into one (count + web history), halving latency and API traffic - make ftp_over_https default to NULL consistently in geo_suppl() - fix the wrong slot name in the GEOPlatform validity error - document print.GEOSoft and unexport the internal soft_print_* S3 methods (R CMD check no longer sees undocumented S3 methods) - document geo_soft() list return and class dispatch - flatten anyhow error chains into single-line R conditions instead of multi-line Debug dumps - fix 'meta(gpl)' in the provenance script (no such function) - move stats to Imports (used unconditionally by log_trans) and drop the unused R.utils from Suggests
- the review wrongly flagged R.utils as unused: data.table::fread() loads it dynamically to read gz files, and the geo-suppl vignette does exactly that, so removing it broke vignette rebuilding in CI - the live accessor test called gpl() on a GEOSoft (GSM) object, for which no method exists; it only ever ran where the network was blocked, so the mistake surfaced first in CI. Assert rcd_name/rcd_type on the GSM object itself instead
Codecov Report❌ Patch coverage is
📢 Thoughts on this report? Let us know! |
- add a CONTRIBUTING.md covering the Rust toolchain requirement, PR and testing conventions, and issue reporting - record the new vignettes, the parse_sample_data fix, and the CI querychat workaround in NEWS.md - SKILL.md: querychat needs duckdb or RSQLite, not duckdb specifically - ignore CONTRIBUTING.md in the R build
…docs - the new platform_id guard and the annotation-failure alert interpolated an 'accession' that parse_gse_matrix does not bind (it resolved to the S4 generic, crashing cli); derive it from the matrix file name instead - cherry-pick the docs commit that was missing from this branch: CONTRIBUTING.md, NEWS.md entries, SKILL.md engine note, CONTRIBUTING Rbuildignore entry - reword the geo_gtype malformed-input test to use genuinely non-coercible input - keep the soft_print_* S3 registrations at main parity and fix the success-code comment
The padding width for non-matching elements was derived from the first element of the regexec result, but a non-match reports a single -1 position, so a lone non-matching input degraded to a 1x1 result and still crashed downstream [, 2L] indexing (stringr returns one column per capture group). Derive the width from the first element that actually matched; when nothing matches the result degrades to a single all-NA column, which regexec cannot attribute to capture groups. parse_characteristics() now falls back to the raw column name as the prefix for custom `fields` without a "ch<N>" suffix instead of assuming group two exists. Empirically reproducible before the fix: parse_sample_data(df, fields = "custom_col") # Error: subscript out of bounds
The FTP success-detection regression, the annotation-fallback crash in parse_gse_matrix(), and the parse_sample_data column-selection issues were announced in the PR body but missing from NEWS.md, contrary to the convention CONTRIBUTING.md states.
ubuntu-latest flipped to the 26.04 (resolute) runner image between the 2026-10-25 and 2026-10-26 runs. RSPM does not yet serve an R-devel binary for duckdb on the new resolute repo, so pak fell back to a 59-minute source build that then failed to install, failing the ubuntu-latest (devel) job before the check even started. Pin only the devel matrix entry to ubuntu-24.04, where RSPM binary coverage is mature (the green 10-25 devel run installed duckdb from an RSPM binary there). The R-release and oldrel-1 jobs stay on ubuntu-latest: resolute binaries for those R versions exist and both jobs passed on 2026-10-26, so pinning them would only invalidate their caches.
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
Follow-up to #8 (its landing-page/README part was merged quickly; this PR carries the systematic-review fixes). A full independent code review (all 16 R files, Rust interface, tests, infra) plus a docs/tests/infra audit found the following, all verified empirically and fixed here:
Major fixes
R/download.R): success detection regressed to requiring HTTP 200, but plain-FTP transfers complete with 226 (206/416 ranged) — successful downloads were deleted and a spurious error raised. Restored the documented multi-status check.R/geo-matrix.R): undefined variabledownloaded(should be the annotation download result), plus raw cli markup glued viapaste().parse_sample_data()(R/phenodata.R): accidental logical-index recycling skipped valid non-contiguouscharacteristics_chcolumns.str_match()crashed on non-matching input (R/utils-str.R): no-match collapsed the result to one column, breaking downstream[, 2L]; now returns stringr-compatible all-NA rows.Also fixed (from the review)
parse_sample_data()ignored itssepargument during parsing (hardcoded ":").geo_search(): validatequery; merge twoentrez_search()calls into one (count + web history).geo_matrix_impl()dead defaults aligned with the wrapper; clear error when sample metadata lacksplatform_id.GEOPlatformvalidity error named the wrong slot;print.GEOSoftdocumented, internalsoft_print_*S3 methods unexported (R CMD check clean).geo_suppl()ftp_over_httpsdefault unified toNULL; typos; 5 unusedstr_*helpers pruned;statsmoved to Imports. (R.utilswas briefly removed as apparently unused but restored:data.table::fread()loads it dynamically for gz files, which the geo-suppl vignette relies on.)Tests
New test files cover previously untested exports (11 of 22 exports had zero coverage):
log_trans,parse_sample_data,geo_gtype/geo_url,geo_search,geo_meta, and the GEOSoft record accessors. Network tests follow the package's skip-on-failure convention;geo_searchgets a network-aware skip since it bypasses the download layer.Docs/infra
sepfix, and the CI querychat workaround.@return; man pages regenerated (roxygen2 8.1.1).Validation
R CMD check(--as-cran, --no-vignettes): no new errors/warnings/notes vs main; remaining items are pre-existing network artifacts of the check environment.cargo checkpasses for the Rust change.