Repository navigation
GDAL Configuration System (#8) - #15
Merged
Merged
Conversation
Three-tier design with GDAL as the single source of truth: - value: gdal_config() - composable bundle of global config opts + path-bound VSI opts, with constructor/coercions/c() merge/cli print and as_gdal_args() rendering (vsi omitted with message; flatten_vsi escape hatch with conflict warning) - state: gdal_config_set/get/unset/reset + gdal_config_active(); priors recorded on first touch for exact restore; unset modes (reveal/mask/scrub) encode the envvar-fallback asymmetry; scoped local_/with_gdal_config() restore values and state even on error - view: gdal_config_sitrep() live provenance report (#12 initial scope) with at-load baseline stash Also: gdalrc I/O (#13) with full grammar incl. [credentials] path bindings and as_gdal_config() bridge; runtime-derived known-option universe (VSI fs options + driver metadata + curated core list); fill-only tracked GDAL_HTTP_USERAGENT load default with opt-out; config conditions/predicates and shared cli_redact().
![gemini-code-assist[bot]](https://avatars.githubusercontent.com/in/956858?s=60&v=4){kind=small}
There was a problem hiding this comment.
Code Review
This pull request introduces a comprehensive GDAL configuration management system, including verbs for setting, getting, unsetting, and resetting configurations, scoped configurations, situational reports, and gdalrc file I/O. Feedback on the changes highlights that the internal .gdalrc_entries function introduces an undeclared dependency on the tidyr package, and suggests a dependency-free alternative using base R and already-imported packages.
Important
The consumer version of Gemini Code Assist on GitHub is being sunset. Starting June 18, 2026, new organization installations will be blocked, and all code review activity will officially cease on July 17, 2026.
For more details on the timeline and next steps, please review the Help Documentation.
Comment on lines
+253
to
+281
| .gdalrc_entries <- function(path) { | ||
| tibble::tibble(raw = stringr::str_trim(readLines(path, warn = FALSE))) |> | ||
| dplyr::filter(nzchar(.data$raw), !stringr::str_starts(.data$raw, stringr::fixed("#"))) |> | ||
| dplyr::mutate( | ||
| is_subsection = stringr::str_detect(.data$raw, "^\\[\\..+\\]$"), | ||
| is_section = !.data$is_subsection & stringr::str_detect(.data$raw, "^\\[.+\\]$"), | ||
| section = dplyr::if_else( | ||
| .data$is_section, | ||
| tolower(stringr::str_remove_all(.data$raw, "^\\[|\\]$")), | ||
| NA_character_ | ||
| ), | ||
| subsection = dplyr::if_else( | ||
| .data$is_subsection, | ||
| stringr::str_remove_all(.data$raw, "^\\[\\.|\\]$"), | ||
| NA_character_ | ||
| ) | ||
| ) |> | ||
| tidyr::fill("section") |> | ||
| dplyr::group_by(.data$section) |> | ||
| tidyr::fill("subsection") |> | ||
| dplyr::ungroup() |> | ||
| dplyr::filter(!.data$is_section, !.data$is_subsection, stringr::str_detect(.data$raw, stringr::fixed("="))) |> | ||
| tidyr::separate_wider_delim("raw", "=", names = c("key", "value"), too_many = "merge") |> | ||
| dplyr::mutate( | ||
| key = stringr::str_trim(.data$key), | ||
| value = stringr::str_trim(.data$value) | ||
| ) |> | ||
| dplyr::select("section", "subsection", "key", "value") | ||
| } |
There was a problem hiding this comment.
The internal .gdalrc_entries function introduces a dependency on the tidyr package (specifically tidyr::fill and tidyr::separate_wider_delim). However, tidyr is not imported in the package's NAMESPACE, which could lead to runtime failures if the package is used in an environment where tidyr is not installed. Even if it is installed, adding a heavy dependency like tidyr for a single internal helper function is unnecessary. We can easily replace these operations with lightweight, dependency-free base R and dplyr/stringr equivalents (which are already imported and used).
.gdalrc_entries <- function(path) {
fill_down <- function(x) {
if (length(x) <= 1L) return(x)
Reduce(function(a, b) if (is.na(b)) a else b, x, accumulate = TRUE)
}
tibble::tibble(raw = stringr::str_trim(readLines(path, warn = FALSE))) |>
dplyr::filter(nzchar(.data$raw), !stringr::str_starts(.data$raw, stringr::fixed("#"))) |>
dplyr::mutate(
is_subsection = stringr::str_detect(.data$raw, "^\\[\\..+\\]$"),
is_section = !.data$is_subsection & stringr::str_detect(.data$raw, "^\\[.+\\]$"),
section = dplyr::if_else(
.data$is_section,
tolower(stringr::str_remove_all(.data$raw, "^\\[|\\]$")),
NA_character_
),
subsection = dplyr::if_else(
.data$is_subsection,
stringr::str_remove_all(.data$raw, "^\\[\\.|\\]$"),
NA_character_
)
) |>
dplyr::mutate(section = fill_down(section)) |>
dplyr::group_by(.data$section) |>
dplyr::mutate(subsection = fill_down(subsection)) |>
dplyr::ungroup() |>
dplyr::filter(!.data$is_section, !.data$is_subsection, stringr::str_detect(.data$raw, stringr::fixed("="))) |>
dplyr::mutate(
key = stringr::str_trim(sub("^([^=]+)=.*$", "\\1", .data$raw)),
value = stringr::str_trim(sub("^[^=]+=(.*)$", "\\1", .data$raw))
) |>
dplyr::select("section", "subsection", "key", "value")
}
This was
linked to
issues
Jul 2, 2026
There was a problem hiding this comment.
Pull request overview
Implements a three-tier GDAL configuration system for gdalvector, providing (1) inert, composable configuration values (gdal_config), (2) stateful verbs to apply/restore configuration in-process, and (3) a provenance “sitrep” view plus gdalrc read/write support.
Changes:
- Adds
gdal_configvalue class (global + path-bound VSI channels), rendering (as_gdal_args()), composition (c()), and coercions. - Introduces stateful configuration verbs (
set/get/unset/reset), scoped helpers (local_gdal_config()/with_gdal_config()), and a provenance report (gdal_config_sitrep()), including load-time defaults. - Adds gdalrc I/O (
gdal_config_file_read()/write()/discover()) and extensive test coverage.
Reviewed changes
Copilot reviewed 10 out of 20 changed files in this pull request and generated 5 comments.
Show a summary per file
| File | Description |
|---|---|
| tests/testthat/test-gdal_config.R | Adds comprehensive tests for value/state/view tiers, envvar modes, gdalrc round-tripping, and defaults. |
| R/zzz.R | Runs config defaults initialization at package load after driver metadata init. |
| R/utils_predicates.R | Expands predicate docs; updates is_vsi_path() behavior and adds config-related predicates. |
| R/utils_cli.R | Adds cli_redact() utility for key-based secret redaction in printed output. |
| R/gdalvector-conditions.R | Adds config-scoped condition wrappers (gdal_*_config). |
| R/gdal_vsi.R | Adds lazy runtime enumeration/caching of VSI option metadata (.vsi_fs_options_tbl()). |
| R/gdal_config.R | Implements the gdal_config system: value class, stateful verbs, scoped helpers, sitrep, defaults, and internal bookkeeping. |
| R/gdal_config_file.R | Adds gdalrc parsing/serialization/discovery and coercion to gdal_config. |
| R/aaa.R | Adds curated GDAL_VSI_PREFIXES and GDAL_CORE_CONFIG_OPTS lists for known-option coverage. |
| NAMESPACE | Exports new APIs and registers new S3 methods. |
| man/*.Rd | Adds/updates generated documentation for new config system APIs and predicates. |
Files not reviewed (10)
- man/as_gdal_args.Rd: Generated file
- man/as_gdal_config_opts.Rd: Generated file
- man/gdal_config.Rd: Generated file
- man/gdal_config_active.Rd: Generated file
- man/gdal_config_file.Rd: Generated file
- man/gdal_config_set.Rd: Generated file
- man/gdal_config_sitrep.Rd: Generated file
- man/is_vsi_path.Rd: Generated file
- man/predicates.Rd: Generated file
- man/with_gdal_config.Rd: Generated file
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
Comment on lines
57
to
63
| is_vsi_path <- function(x) { | ||
| if (!is.character(x) || length(x) != 1L || !nzchar(x)) { | ||
| return(FALSE) | ||
| } | ||
| all(startsWith(x, "/vsi"), grepl("^/vsi[a-z0-9_]+/", x, ignore.case = TRUE)) | ||
| # all(startsWith(x, "/vsi"), grepl("^/vsi[a-z0-9_]+/", x, ignore.case = TRUE)) | ||
| any(startsWith(x, GDAL_VSI_PREFIXES)) | ||
| } |
Comment on lines
+98
to
+101
| gdal_config_file_read <- function(path) { | ||
| check_path(path) | ||
| entries <- .gdalrc_entries(path) | ||
|
|
Comment on lines
+628
to
+631
| #' Coverage spans keys pinned by the package, GDAL-relevant environment variables, config-file | ||
| #' entries, and a probe of the full known-option universe (runtime VSI metadata + driver metadata | ||
| #' + curated core names), so externally set in-memory values for any documented key are discovered | ||
| #' too. Secret-bearing values are redacted in printed output. A baseline sitrep taken at package |
- terminate the predicates roxygen topic with NULL so it no longer absorbs is_int64() (undocumented-argument warning in predicates.Rd) - fix two typos (caes/Inheritence) and update inst/WORDLIST R CMD check: 0 errors, 0 warnings, 0 notes
…tions - restore pattern-based is_vsi_path() (hardcoded prefix list was case-sensitive and missed 19 of the 32 handlers the running build registers); GDAL_VSI_PREFIXES stays as the curated common set - gdal_config_file_read() errors immediately (classed) on a directory - fix sitrep roxygen line misparsed as a markdown list item - per-function @importFrom tags for all ::-used externals (rlang pronouns/operators stay package-wide) and @returns content on the line below the tag, per package conventions
Owner
Author
|
Review comments triaged (4df3393 + this commit): Addressed:
Rejected:
Also in this commit: a style pass aligning the new config files with package roxygen conventions (per-function |
This was
unlinked from
issues
Jul 2, 2026
The commented block in gdal_sitrep.R listed various functions from gdalraster, sf, terra, vapour, and geos. This list appears to be a vestige of early development and is no longer needed, improving code hygiene.
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
Implements the stateful GDAL configuration system designed in #8, structured as three tiers with GDAL itself as the single source of truth (nothing mirrored):
gdal_config(): an inert, composable bundle of agdal_config_optspayload (global channel) + path-boundgdal_vsi_opts(VSI channel), with full S3 conventions (new_gdal_config(),as_gdal_config()coercions,c()merge with later-wins semantics, cliformat()/print()).as_gdal_args()renders the global channel as--configtokens and omits path-bound VSI options with a message (they cannot ride the CLI);flatten_vsi = TRUEopts into lossy flattening with a conflict warning.gdal_config_set()/get()/unset()/reset()apply values to GDAL (set_config_option()/vsi_set_path_option()), track the session's active configuration (gdal_config_active()), and record priors on first touch soreset()always restores the true pre-package state.local_gdal_config()/with_gdal_config()provide exact scoped application (GDAL values and package state restored, even on error) - the recommended pattern around pipeline executions.gdal_config_unset(mode = c("reveal", "mask", "scrub"))makes the envvar-fallback asymmetry explicit (set is authoritative; unset merely reveals).gdal_config_sitrep(): live provenance report (gdalvector/envvar/config_file/external/unsetper key), probing the full known-option universe; anat_loadbaseline sitrep is stashed at package load ([Feature]: gdal_config_sitrep() - Configuration Situational Report #12, initial scope).Also included:
gdal_config_file_read()/write()with the full grammar ([configoptions],[directives],[credentials]with[.subsection]/pathbindings ->gdal_vsi_optsvalues);as_gdal_config()bridges a parsed file into the configuration system;gdal_config_file()discovers/parses the file GDAL loaded at init. The config file is the only lossless serialization of a full configuration (path-bound credentials have no CLI representation)..vsi_fs_options_tbl()), the curated per-driver config metadata, and a small curatedGDAL_CORE_CONFIG_OPTScore list (the only part GDAL does not expose at runtime). Advisory (classed, non-blocking) unknown-key warnings.GDAL_HTTP_USERAGENT(gdalvector/x.y.z), never overriding user env/config, opt-out viaoptions(gdalvector.config_defaults = FALSE).gdal_*_config()conditions,is_gdal_config*()predicates, sharedcli_redact()(secrets never printed), corrected precedence notes indev/ref/gdal_config_nuances_examples.R.Closes #8. Closes #13. Part of #12 (initial scope; per-prefix credential resolution and diffing follow). Groundwork for #14 (presets compose via
c.gdal_config).Test plan
devtools::document()clean; NAMESPACE regenerated.