fix: make ggExpress an optional (Suggests) dependency for qbarplot() - #46
Merged
Conversation
df.remove.empty.rows.and.columns() called bare qbarplot(), which is
defined in the ggExpress package -- not a declared dependency of
CodeAndRoll2 (DESCRIPTION only declares Depends: Stringendo). Unlike
the other two undeclared-dependency bugs fixed in this batch, this
one crashed on the function's own *default* arguments
(plot_stats = TRUE by default), not just an opt-in code path, making
it the most severe of the three.
ggExpress cannot simply become a hard Import here: per the package's
own README, ggExpress is downstream of CodeAndRoll2 (built on top of
it), so a hard dependency in this direction would create a circular
package dependency.
Fixed by guarding the call with
requireNamespace("ggExpress", quietly = TRUE): if ggExpress is
installed, the plot renders exactly as before via
ggExpress::qbarplot(...); if not, the function now prints an
informative message and skips the plot instead of erroring. Declared
ggExpress under Suggests in DESCRIPTION/config.R (a soft dependency
that doesn't force install order or risk circularity) and updated
the roxygen docs to note this.
Verified: df.remove.empty.rows.and.columns(df) with default arguments
(ggExpress not installed in the test environment) now completes
successfully with a clear message, instead of
"could not find function 'qbarplot'".
![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: 15d220edc7
ℹ️ 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.
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 |
Co-authored-by: vertesy <5101911+vertesy@users.noreply.github.com>
Contributor
Resolved the merge conflicts by merging |
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?
df.remove.empty.rows.and.columns()(an exported function) calls bareqbarplot(...)whenplot_stats = TRUE.qbarplotis defined in the ggExpress package, which is not declared anywhere inDESCRIPTION(Depends: Stringendoonly).This is the most severe of the three undeclared-dependency bugs found in this audit batch, because
plot_statsdefaults toTRUE— the crash fires on ordinary, argument-free use, not just an opt-in code path.Why couldn't
ggExpressjust become a normal Import?Per the package's own
README.md,ggExpressis downstream ofCodeAndRoll2— it's built on top of it, not the other way around. Declaring it as a hardDepends/Importshere would create a circular package dependency (CodeAndRoll2→ggExpress→CodeAndRoll2), which R does not support cleanly.The fix
Made
ggExpressa soft, optional dependency:if (requireNamespace("ggExpress", quietly = TRUE)) { ggExpress::qbarplot(...) } else { message(...) }— ifggExpressis installed, the plot renders exactly as before; if not, the function now prints one informative message and skips the plot instead of crashing.ggExpressunderSuggests:inDESCRIPTION(and the upstreamDevelopment/config.Rsource it's generated from) —Suggestsdoesn't force install order and carries no circularity risk.plot_statsand...to note the optional dependency.What was the impact of the old (wrong) behavior?
Any call to
df.remove.empty.rows.and.columns(df)— with no other arguments, sinceplot_stats = TRUEis the default — threw:unless the caller happened to have
ggExpressseparately installed and attached.Testing
Before:
Error in qbarplot(...) : could not find function "qbarplot".After:
— completes successfully with a clear message instead of crashing, and the data-frame-filtering logic (the function's core job) is unaffected either way.
R CMD build+R CMD check(with_R_CHECK_FORCE_SUGGESTS_=false, since the sandbox doesn't have the now-optionalggExpressinstalled — the same env var a maintainer would use for any newly-added optional Suggests not yet present locally): the "no visible global function definition for 'qbarplot'" NOTE is gone; no new NOTEs/WARNINGs/ERRORs introduced by this change (remaining output is pre-existing and unrelated — the other two undeclared-dependency crashes tracked in separate PRs, and long-standing missing base-packageimportFromentries).man/df.remove.empty.rows.and.columns.Rdviaroxygen2::roxygenise(".").testthat/automated tests added, per repo convention.Generated by Claude Code