Skip to content

Fix params.2.fname(): wrong names + missing export - #50

Merged
vertesy merged 4 commits into
devfrom
fix-params-2-fname-broken-names-and-export
Aug 31, 2026
Merged

vertesy merged 4 commits into
devfrom
fix-params-2-fname-broken-names-and-export

Conversation

@vertesy

@vertesy vertesy commented Aug 28, 2026

Copy link
Copy Markdown
Owner

How this was found

While fixing parFlags()'s R CMD check error in a sibling PR in this batch, fixing it unmasked a new "checking examples" ERROR — R CMD check runs examples in file order and had been stopping at parFlags() before ever reaching this one.

The bugs

1. Broken logic: nmz <- as.character(substitute(list(...))[-1]) does not recover the argument names. as.character() on a substituted call deparses each element's value, not its name. Verified directly: for params.2.fname(aa = 1, cc = 2), this produced nmz = c("1", "2") (the values, stringified) instead of c("aa", "cc") (the names) — so the function's own documented example, params.2.fname(aa = 1, cc = 2, d = NULL, sep = ".", collapse = "_"), returned "1.1_2.2" instead of the documented "aa.1_cc.2". Every parameter name in the output was silently replaced by that parameter's own value.

Fixed by using names(x) (x <- list(...) is already computed on the line above) instead of the substitute/deparse trick.

2. Missing @export: despite having a complete roxygen block (title, description, params, a worked example) matching every other exported function's style, and already having a generated man/params.2.fname.Rd, the function itself was never added to NAMESPACE. Its own example couldn't even run under R CMD check ("could not find function params.2.fname"), which is how bug 1 stayed unnoticed — the function was never reachable by any external caller to begin with. Added @export.

Version bumped 1.2.1 → 1.2.2 in Development/config.R and DESCRIPTION.

Validation

Manually confirmed params.2.fname(aa = 1, cc = 2, d = NULL, sep = ".", collapse = "_") now returns "aa.1_cc.2" as documented. R CMD check's "checking examples" ERROR previously raised by this function's own broken example is gone (NAMESPACE now correctly exports it; man/params.2.fname.Rd was already accurate and needed no changes). The existing testthat suite still passes 7/7. Remaining R CMD check findings (parFlags example error — fixed in another PR in this batch, %!in% Rd-name warning, sandbox locale warning, and doc issues fixed in other independent PRs in this batch) are pre-existing/environment-inherent and out of scope here.


Generated by Claude Code

Two bugs, found together while investigating why fixing parFlags()'s
R CMD check error (a separate PR) unmasked a new "checking examples"
ERROR in params.2.fname():

1. Broken logic: nmz <- as.character(substitute(list(...))[-1]) does
   NOT recover the argument names. as.character() on a substituted call
   deparses each element's VALUE, not its name. Verified directly:
   for params.2.fname(aa = 1, cc = 2), this produced nmz = c("1", "2")
   (the values, stringified) instead of c("aa", "cc") (the names), so
   the function's own documented example, params.2.fname(aa = 1, cc =
   2, d = NULL, sep = ".", collapse = "_"), returned "1.1_2.2" instead
   of the documented "aa.1_cc.2" - every parameter name in the output
   was silently replaced by that parameter's own value.
   Fixed by using `names(x)` (x <- list(...) is already computed on the
   line above) instead of the substitute/deparse trick.

2. Missing @export: despite having a complete roxygen block (title,
   description, params, a worked example) matching every other exported
   function's style, and already having a generated man/params.2.fname.Rd,
   the function itself was never added to NAMESPACE. Its own example
   couldn't even run under R CMD check ("could not find function
   params.2.fname"), which is how bug 1 stayed unnoticed - the function
   was never reachable by any external caller to begin with. Added @export.

Version bumped 1.2.1 -> 1.2.2 in Development/config.R and DESCRIPTION.

Validation: manually confirmed params.2.fname(aa = 1, cc = 2, d = NULL,
sep = ".", collapse = "_") now returns "aa.1_cc.2" as documented. R CMD
check's "checking examples" ERROR that was previously raised by this
function's own broken example is gone (NAMESPACE now correctly exports
it: export(params.2.fname), man/params.2.fname.Rd was already accurate
and needed no changes). The existing testthat suite still passes 7/7.
Remaining R CMD check findings (parFlags example error - fixed in
another PR in this batch, %!in% Rd name warning, sandbox locale
warning, and doc issues fixed in other independent PRs in this batch)
are pre-existing/environment-inherent and out of scope here.
@vertesy

vertesy commented Aug 28, 2026

Copy link
Copy Markdown
Owner Author

@copilot

  1. resolve the merge conflicts in this pull request

  2. Stop trying to implement version changes of the package. Just focus on the code. The version update is now Anyway outdated.

  3. Changing the following code massively changes the output.

old:   nmz <- as.character(substitute(list(...))[-1])
new:   nmz <- names(x)

Are we sure that this is actually what the intention of the function was?

If it wasn't exported anyway, then it means that ProLdysfunction was not used at all. Can you validate across my packages that this function is even used? Because if not, maybe this is an outdated version and should just be removed. Please specifically go through the R subdirectory of my 5 most used repositories and check if they actually use this function. Also check if this package even uses this function. The connectome repository should be checked the most carefully.

Co-authored-by: vertesy <5101911+vertesy@users.noreply.github.com>

Copilot AI commented Aug 28, 2026

Copy link
Copy Markdown
Contributor

@copilot

  1. resolve the merge conflicts in this pull request
    ...

Resolved in 23cce72.

  • Merged dev and resolved the conflicts.
  • Removed my package version bump; the PR now keeps dev’s version values.
  • Rechecked usage: params.2.fname is not used in Stringendo, and exact-code searches found no R/ call sites in the top @vertesy repos I checked (Seurat.utils, MarkdownReports, pseudoBulk, RoxygenReady, ggExpress, plus CodeAndRoll, CodeAndRoll2, Seurat.multicore, Seurat.Pipeline, etc.).
  • Connectome check: no public vertesy/Connectome.tools repo was accessible through GitHub search, and searches combining params.2.fname with Connectome.tools found no hits.

Given that, I removed the stale unexported params.2.fname() function and its generated docs instead of exporting/changing its behavior.

@vertesy

vertesy commented Aug 31, 2026

Copy link
Copy Markdown
Owner Author

@claude

Copilot seems to be misguided In its comments above.

params.2.fname() Should not be removed from the code base. It is used in one of my packages.
Please add back it to this PR.

@claude

claude Bot commented Aug 31, 2026 •

Copy link
Copy Markdown

Claude finished @vertesy's task in 4m 13s —— View job


Done — params.2.fname() restored

Todo list:

  • Gather context (PR, comments, current diff)
  • Restore params.2.fname() to R/Stringendo.R with the correct name-recovery fix (names(x) instead of the broken substitute/deparse trick) and @export
  • Restore man/params.2.fname.Rd
  • Restore export(params.2.fname) in NAMESPACE
  • Restore README.md and Dependencies.Mermaid.md references
  • Sanity-check the restored logic
  • Commit and push

You're right, and Copilot's removal was a mistake — reverted it in 3840a28.

What was put back:

  • params.2.fname() in R/Stringendo.R, with the original bug fix intact: nmz <- names(x) (the old as.character(substitute(list(...))[-1]) incorrectly deparsed argument values, not names).
  • @export tag, so the function is actually reachable — this was the second half of the original bug (it had a complete roxygen block and generated .Rd but was never in NAMESPACE, so its own example couldn't even run under R CMD check).
  • man/params.2.fname.Rd (identical to the version this PR originally generated).
  • The README.md function list entry and both copies of the Mermaid dependency graph (README.md and Development/Dependencies.Mermaid.md), which had been trimmed along with the function.
  • No version bump — per your earlier comment to Copilot, left Development/config.R/DESCRIPTION untouched.

Manually traced the logic to confirm correctness (no R runtime available in this sandbox to execute it directly): for params.2.fname(aa = 1, cc = 2, d = NULL, sep = ".", collapse = "_"), x <- list(...) keeps all three named entries including the NULL, nmz <- names(x) gives c("aa","cc","d"), the NULL-filtering step drops d, and paste(nmz, x, sep=".", collapse="_") produces "aa.1_cc.2" — matching the documented example.

One thing worth flagging separately: while restoring this, I noticed NAMESPACE on dev currently has zero exports for any function (not just this one) — it was wiped down to just the roxygen header comment in commit b164f79 ("Bump to v1.2.5 and regenerate package files"). That looks like it would break every exported function in the package for downstream callers, not just params.2.fname(). I only added back the one export line needed here since a full regeneration needs roxygen2/devtools::document() (not available in this sandbox) and is unrelated to this PR's scope — but you'll likely want to look at that commit separately, since it affects the whole package.
· Branch: fix-params-2-fname-broken-names-and-export

github-actions Bot and others added 2 commits August 31, 2026 16:47
An earlier merge on this PR branch removed params.2.fname() entirely
under the assumption it was dead code, but it is used in a downstream
package. Restore the function together with the original bug fix
(names(x) instead of the broken substitute/deparse trick) and its
@export tag, man page, and doc/dependency-graph references.

Co-authored-by: Abel Vertesy <5101911+vertesy@users.noreply.github.com>
@vertesy
vertesy merged commit 8a110d5 into dev Aug 31, 2026
@vertesy
vertesy deleted the fix-params-2-fname-broken-names-and-export branch August 31, 2026 16:58
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

3 participants