fix: inline FirstCol2RowNames() logic to remove undeclared dependency - #45
Merged
Merged
Conversation
merge_ls_of_named_vec_as_df_cols() called bare FirstCol2RowNames(), which is defined in the ReadWriter package. ReadWriter is not declared anywhere in DESCRIPTION (Depends: Stringendo only), so any call to this exported function threw "could not find function 'FirstCol2RowNames'" unless the caller happened to have ReadWriter separately loaded. Fixed by inlining the (small, unambiguous) "move the first column's values into row names, then drop that column" logic directly, rather than declaring ReadWriter as a new dependency. CodeAndRoll2 is the most upstream, foundational @vertesy package (per its own README and AGENTS.md), and ReadWriter itself likely depends on it -- adding ReadWriter as an Import here would risk a circular package dependency. Inlining avoids that risk entirely. Verified against the function's own default arguments: merge_ls_of_named_vec_as_df_cols() now returns the expected merged data frame with correct row names instead of erroring.
![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: 74d973ab42
ℹ️ 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.
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).
vertesy
pushed a commit
that referenced
this pull request
Aug 28, 2026
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. |
…undeclared-dependency # 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?
merge_ls_of_named_vec_as_df_cols()(an exported function) ends withreturn(FirstCol2RowNames(COMBINED)).FirstCol2RowNamesis defined in the ReadWriter package.CodeAndRoll2'sDESCRIPTIONonly declaresDepends: Stringendo—ReadWriteris not a dependency anywhere (notDepends, notImports).Why did it happen?
At this point in the function,
COMBINEDis a plain data frame produced by repeatedlymerge()-ingstack()-ed named vectors on their shared key column"ind"— so its first column holds the union of all the input vectors' names, and the rest are the merged value columns.FirstCol2RowNames()is presumably a ReadWriter convenience for "take the first column's values and use them as row names," which is a well-defined, unambiguous operation — but calling it here silently assumed ReadWriter would be available.What was the impact of the old (wrong) behavior?
Calling
merge_ls_of_named_vec_as_df_cols()— including with no arguments at all, since the function has full default values for every parameter — threw:for any user who did not separately have
ReadWriterloaded.The fix
Inlined the "move first column's values into row names, then drop that column" logic directly:
instead of adding
ReadWriteras a new dependency.CodeAndRoll2is the most upstream, foundational@vertesypackage (per its ownREADME.mdandAGENTS.md) andReadWriteritself likely depends on it — declaringReadWriteras anImporthere would risk a circular package dependency. Inlining sidesteps that entirely and needs no external call. Happy to reconsider if the repo owner would rather declare the dependency instead.Testing
merge_ls_of_named_vec_as_df_cols() # uses the function's own default argumentsBefore:
Error in FirstCol2RowNames(COMBINED) : could not find function "FirstCol2RowNames".After: returns the expected merged data frame —
with
rownames(res)correctly"A" "B" "C" "D" "E".R CMD build+R CMD check: the "no visible global function definition for 'FirstCol2RowNames'" NOTE is gone; no new NOTEs/WARNINGs/ERRORs introduced (remaining output is pre-existing and unrelated — the other undeclared-dependency crash tracked in a separate PR, and long-standing missing base-packageimportFromentries)..Rd/NAMESPACEchange needed.testthat/automated tests added, per repo convention.Generated by Claude Code