The function config::get(config = "some-name") does not check that some-name is present in the file. An
unrecognised name yields the default profile, with no error and no warning, so asking for a
profile that does not exist is indistinguishable from asking for default.
Because default is conventionally the production profile, the failure mode is that a typo
quietly selects production and returns plausible results.
Reproducible example
d <- tempfile(); dir.create(d); setwd(d)
writeLines(c(
"default:",
" dsn: PRODUCTION-DATABASE",
"staging:",
" dsn: STAGING-DATABASE"
), "config.yml")
for (p in c("default", "staging", "stagng", "STAGING", "", "nonexistent")) {
cat(sprintf(" config = %-13s -> %s\n", encodeString(p, quote = '"'),
config::get("dsn", config = p)))
}
config = "default" -> PRODUCTION-DATABASE
config = "staging" -> STAGING-DATABASE
config = "stagng" -> PRODUCTION-DATABASE
config = "STAGING" -> PRODUCTION-DATABASE
config = "" -> PRODUCTION-DATABASE
config = "nonexistent" -> PRODUCTION-DATABASE
Only the first two names exist in the file. The other four all return the production value.
Profile names are also case-sensitive, so "STAGING" misses.
R_CONFIG_ACTIVE takes the same path, which is the variant most likely to reach production:
R_CONFIG_ACTIVE="staging" -> STAGING-DATABASE
R_CONFIG_ACTIVE="stagng" -> PRODUCTION-DATABASE
R_CONFIG_ACTIVE="" -> PRODUCTION-DATABASE
A mistyped deployment variable does not fail the job; it points it at default.
config 0.3.2, R 4.5.2.
Why this is worth an error
Beyond the typo case, two situations where the silence is expensive:
- Renaming a profile. Rename
staging to stage and miss a call site: that call site keeps
working, against default, rather than failing. The refactor cannot be verified by running the
code, because the wrong answer is a successful answer.
- Deployment.
R_CONFIG_ACTIVE is typically set once in an environment that is not part of
the code under review, so the mistake is not visible where people are looking.
Nothing in the returned value indicates which profile produced it.
Mechanism
do_get <- function(config, inherited = c()) {
...
active_config <- config_yaml[[config]]
...
}
active_config <- merge_lists(default_config, do_get(config))
config_yaml[[config]] is NULL for a name that is not a key in the file, and
merge_lists(x, NULL) returns x, so the result is default_config unchanged. The requested
name is never compared against names(config_yaml).
A missing file is already an error, with a good message about parent directories. A missing
profile is not, which reads more like an oversight than a decision.
Suggested fix
Error when the requested name is absent from the file, using the known valid set in the message:
Unknown configuration 'stagng'. config.yml defines: default, staging.
Two things to decide:
inherits: targets would want the same validation, or the silence just moves down a level.
- Backward compatibility: code may exist that relies on an unknown name meaning "the default",
though that seems more likely to be latent bugs than deliberate use. A warning first, an error
in a later release, would be the gentler route.
This would also resolve the ambiguity behind #23 ("any way to know if entry is default?"), which
is the same problem from the other direction: today there is no way to tell "I asked for default"
apart from "I asked for something that does not exist".
Happy to send a PR if the approach and the compatibility call suit you.
The function
config::get(config = "some-name")does not check thatsome-nameis present in the file. Anunrecognised name yields the
defaultprofile, with no error and no warning, so asking for aprofile that does not exist is indistinguishable from asking for
default.Because
defaultis conventionally the production profile, the failure mode is that a typoquietly selects production and returns plausible results.
Reproducible example
Only the first two names exist in the file. The other four all return the production value.
Profile names are also case-sensitive, so
"STAGING"misses.R_CONFIG_ACTIVEtakes the same path, which is the variant most likely to reach production:A mistyped deployment variable does not fail the job; it points it at
default.config0.3.2, R 4.5.2.Why this is worth an error
Beyond the typo case, two situations where the silence is expensive:
stagingtostageand miss a call site: that call site keepsworking, against
default, rather than failing. The refactor cannot be verified by running thecode, because the wrong answer is a successful answer.
R_CONFIG_ACTIVEis typically set once in an environment that is not part ofthe code under review, so the mistake is not visible where people are looking.
Nothing in the returned value indicates which profile produced it.
Mechanism
config_yaml[[config]]isNULLfor a name that is not a key in the file, andmerge_lists(x, NULL)returnsx, so the result isdefault_configunchanged. The requestedname is never compared against
names(config_yaml).A missing file is already an error, with a good message about parent directories. A missing
profile is not, which reads more like an oversight than a decision.
Suggested fix
Error when the requested name is absent from the file, using the known valid set in the message:
Two things to decide:
inherits:targets would want the same validation, or the silence just moves down a level.though that seems more likely to be latent bugs than deliberate use. A warning first, an error
in a later release, would be the gentler route.
This would also resolve the ambiguity behind #23 ("any way to know if entry is default?"), which
is the same problem from the other direction: today there is no way to tell "I asked for default"
apart from "I asked for something that does not exist".
Happy to send a PR if the approach and the compatibility call suit you.