fix: disambiguate zero.omit() call from its own shadowing parameter - #48
Merged
Conversation
as.list.df.by.row() and as.list.df.by.col() both take a logical parameter literally named `zero.omit`, then did `lapply(outList, zero.omit)`. Since `zero.omit` is passed as a bare symbol (not called as `zero.omit(...)`), R resolves it to the nearest binding in scope -- the local logical *argument* -- not the package's own exported zero.omit() function. lapply()'s internal match.fun(TRUE) then throws, so calling either function with the explicitly documented `zero.omit = TRUE` crashed. Fixed by calling CodeAndRoll2::zero.omit explicitly in both functions: an explicit pkg::fun reference does a direct namespace lookup and ignores local variable scoping entirely, so it finds the right function regardless of the local parameter name. Applied identically to both functions, which share the exact same bug. Verified: as.list.df.by.row(dtf, zero.omit = TRUE) and as.list.df.by.col(dtf, zero.omit = TRUE) both now return the zero-filtered lists instead of erroring with "'TRUE' is not a function".
![chatgpt-codex-connector[bot]](https://avatars.githubusercontent.com/in/1144995?s=60&v=4){kind=small}
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 4af7a1002f
ℹ️ About Codex in GitHub
Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".
vertesy
pushed a commit
that referenced
this pull request
Aug 28, 2026
Per repo convention (AGENTS.md: bump Development/config.R by 0.0.1 for a substantial code change) and reviewer feedback, so this fix is distinguishable by version from the broken 2.8.1 release. Note: this PR and its siblings in the same batch (#45, #46, #47, #48, #49) each independently bump from the same 2.8.1 base to keep them mergeable in any order; whichever merges first "claims" 2.8.2, and the others may need a trivial one-line renumber at merge time depending on actual merge order.
vertesy
pushed a commit
that referenced
this pull request
Aug 28, 2026
Per repo convention (AGENTS.md: bump Development/config.R by 0.0.1 for a substantial code change) and reviewer feedback. Note: this PR and its siblings in the same batch (#44, #46, #47, #48, #49) each independently bump from the same 2.8.1 base with a distinct target version to keep them mergeable in any order; a trivial one-line renumber may be needed at merge time depending on actual merge order.
vertesy
pushed a commit
that referenced
this pull request
Aug 28, 2026
Per repo convention (AGENTS.md: bump Development/config.R by 0.0.1 for a substantial code change) and reviewer feedback. Note: this PR and its siblings in the same batch (#44, #45, #47, #48, #49) each independently bump from the same 2.8.1 base with a distinct target version to keep them mergeable in any order; a trivial one-line renumber may be needed at merge time depending on actual merge order.
vertesy
pushed a commit
that referenced
this pull request
Aug 28, 2026
…ions Follow-up to the seq_along/seq_len sweep in this PR: reorder.list() and intermingle2lists() both initialized their accumulator as list(NA) (a length-1 sentinel list). Previously, the 1:length(x) version of their loop bound would crash on zero-length input before this sentinel value could ever leak into the return value. Now that the loop correctly does nothing for zero-length input (seq_along(character(0)) / seq_len(0) both being empty), the uninitialized sentinel list(NA) was returned as-is instead of the correct empty list(). Fixed by initializing Lout <- list() in both functions -- list elements are still assigned via Lout[[x]] <- ..., which auto-extends a list on out-of-bounds assignment, so normal (non-empty) behavior is unaffected. Verified: - intermingle2lists(list(a=1,b=2), list(c=3,d=4)) unchanged. - intermingle2lists(list(), list()) now returns list() instead of list(NA). - reorder.list(list(b=2,a=1,c=3)) unchanged. - reorder.list(list(), namesOrdered = character(0)) now returns list() instead of list(NA). Also bumps Development/config.R/DESCRIPTION to 2.8.5 per repo convention and reviewer feedback (a distinct number from sibling PRs #44/#45/#46/#48/#49 in this batch, all bumping from the same 2.8.1 base, so they stay mergeable in any order).
Reviewer feedback on the previous fix: CodeAndRoll2::zero.omit only
works if the package has been formally installed and its namespace
can be loaded. But README.md explicitly documents an alternative,
supported installation method -- sourcing R/CodeAndRoll2.R directly
(no install) -- and in that mode there is no "CodeAndRoll2" namespace
for :: to resolve against, so both functions would still crash with
zero.omit = TRUE for anyone using that method.
Replaced CodeAndRoll2::zero.omit with
get("zero.omit", mode = "function"). get()'s mode argument makes it
skip non-function bindings while searching up the enclosing scope
chain -- so it walks straight past the local logical zero.omit
argument (the actual shadowing problem) and finds the real function,
regardless of whether it's sitting in a loaded package's namespace or
in globalenv() from a direct source() call. Verified both scenarios:
- Package loaded via library(CodeAndRoll2): unchanged, works as
before.
- File sourced directly (source("R/CodeAndRoll2.R"), package not
installed/attached): as.list.df.by.col(dtf, zero.omit = TRUE) now
also works, where CodeAndRoll2::zero.omit would have failed with
"could not find function" (no such namespace loaded).
Also bumps Development/config.R/DESCRIPTION to 2.8.6 per repo
convention and reviewer feedback (a distinct number from sibling
PRs #44/#45/#46/#47/#49 in this batch, all bumping from the same
2.8.1 base, so they stay mergeable in any order).
vertesy
pushed a commit
that referenced
this pull request
Aug 28, 2026
Per repo convention (AGENTS.md: bump Development/config.R by 0.0.1 for a substantial code change) -- applying proactively, matching the same reviewer feedback already given on sibling PRs #44/#45/#46/#47/#48. Note: this PR and its siblings in the same batch each independently bump from the same 2.8.1 base with a distinct target version to keep them mergeable in any order; a trivial one-line renumber may be needed at merge time depending on actual merge order.
Owner
Author
|
@copilot resolve the merge conflicts in this pull request |
Contributor
|
@vertesy Unfortunately I hit an unexpected error while processing your comment. I've automatically reported this to GitHub. You can ask me to try again later by mentioning me in a new comment. If you want to contact GitHub about this error, please mention the following identifier so they can better serve you: Sorry for the inconvenience! |
1 similar comment
Contributor
|
@vertesy Unfortunately I hit an unexpected error while processing your comment. I've automatically reported this to GitHub. You can ask me to try again later by mentioning me in a new comment. If you want to contact GitHub about this error, please mention the following identifier so they can better serve you: Sorry for the inconvenience! |
# Conflicts: # DESCRIPTION # Development/config.R Co-authored-by: vertesy <5101911+vertesy@users.noreply.github.com>
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.
What was the bug?
as.list.df.by.row()andas.list.df.by.col()both take a logical parameter literally namedzero.omit, and both did:Why did it happen?
The bare symbol
zero.omitpassed aslapply'sFUNargument is not a call (zero.omit(...)) — it's just a variable reference. R resolves that reference to the nearest binding in scope, which is the function's own local logical argument (TRUE/FALSE), not the package's exportedzero.omit()function of the same name.lapply()'s internalmatch.fun(TRUE)then throws.What was the impact of the old (wrong) behavior?
Calling either function with the parameter set exactly as documented —
zero.omit = TRUE— crashed:Both functions share the identical bug, since
as.list.df.by.colappears to have been copy-pasted fromas.list.df.by.row(or vice versa).The fix
Called
CodeAndRoll2::zero.omitexplicitly in both functions. An explicitpkg::funreference performs a direct namespace lookup and ignores local variable scoping entirely, so it finds the intended function regardless of what local parameter shares its name. Applied identically to both functions.Testing
Before:
Error in match.fun(FUN) : 'TRUE' is not a function, character or symbolfor both calls.After: both return the expected zero-filtered lists (e.g.
as.list.df.by.colreturnslist(x = c(1,2), y = c(3,4), z = c(5,7))).R CMD build+R CMD check: no new NOTEs/WARNINGs/ERRORs vs. the unmodifieddevbranch — same pre-existing, unrelated issues only (already tracked in other PRs in this batch).NAMESPACE/man/*.Rdviaroxygen2::roxygenise(".")— no actual doc content changed for these two functions since neither the signature nor the roxygen block changed, only the internal call.testthat/automated tests added, per repo convention.Generated by Claude Code