Fix two deprecated wrappers that errored on every real call - #49
Open
vertesy wants to merge 2 commits into
Open
Conversation
Both flag.names_list.all.new() and parFlags() are marked deprecated in
favor of a newer function, but neither actually worked:
- flag.names_list.all.new() was `function() .Deprecated("flag.names_list")`
- it took zero parameters and never called the function it claims to
replace. Any call with an argument failed with "unused argument";
even documented historical usage like flag.names_list.all.new(pl =
p.hm) (its own original signature, see Development/Stringendo.orig.R)
has never worked since it was turned into this stub. Fixed it to
accept ... and forward to flag.names_list(...), matching the
deprecate-and-wrap convention: existing callers keep working (with a
deprecation warning) instead of hitting a hard error.
- parFlags() referenced `namez`, a variable never assigned anywhere in
its scope (only parFlags2() computes it, via
as.character(as.list(match.call())[-(1:2)])). Every call to parFlags()
therefore threw "object 'namez' not found" - including its own
documented @examples. Note this is NOT simply forwarded to parFlags2():
the two functions have different output semantics (parFlags() returns
only the names of TRUE-valued flags, e.g. "MyPlot.pearson.filtered";
parFlags2() returns every name=value pair regardless of truthiness,
e.g. "prefix.pearson_TRUE.filtered_TRUE.normalized_FALSE"), and
forwarding `...` through a wrapper also breaks match.call()'s ability
to recover the original argument names (verified: it degrades to
"..1", "..2", ... inside the callee). So the fix keeps parFlags()'s
own logic intact and gives it the same working name-capture line
parFlags2() already uses, preserving its historical behavior instead
of silently changing what it returns.
Version bumped 1.2.1 -> 1.2.2 in Development/config.R and DESCRIPTION.
Validation: manually confirmed flag.names_list.all.new(ls = ...) now
matches flag.names_list(ls = ...) exactly, and parFlags(prefix =
"MyPlot", pearson, filtered, normalized) now returns
"MyPlot.pearson.filtered" as documented (previously always errored).
R CMD check's "checking examples" ERROR that was previously raised by
parFlags()'s own broken example is gone. The existing testthat suite
still passes 7/7. R CMD check now surfaces a *different*,
previously-masked pre-existing bug (params.2.fname() is missing @export
so its own example fails) - out of scope here, tracked as a follow-up
PR. Remaining findings (%!in% Rd name warning, sandbox locale warning,
flag.names_list.Rd/idate.Rd link warnings and parsepvalue undocumented
prefix from the docs-cleanup/parsepvalue PRs not present on this
independent branch) are pre-existing/environment-inherent and addressed
elsewhere in this batch.
![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: 05a5a62884
ℹ️ 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".
…wrappers # Conflicts: # DESCRIPTION # Development/config.R
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
2 participants
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.
The bugs
Both
flag.names_list.all.new()andparFlags()are marked deprecated in favor of a newer function, but neither actually worked:flag.names_list.all.new()wasfunction() .Deprecated("flag.names_list")— it took zero parameters and never called the function it claims to replace. Any call with an argument failed with "unused argument"; even documented historical usage likeflag.names_list.all.new(pl = p.hm)(its own original signature, seeDevelopment/Stringendo.orig.R) has never worked since it was turned into this stub. Fixed it to accept...and forward toflag.names_list(...), matching the deprecate-and-wrap convention: existing callers keep working (with a deprecation warning) instead of hitting a hard error.parFlags()referencednamez, a variable never assigned anywhere in its scope (onlyparFlags2()computes it, viaas.character(as.list(match.call())[-(1:2)])). Every call toparFlags()therefore threwobject 'namez' not found— including its own documented@examples.Note this is not simply forwarded to
parFlags2(): the two functions have different output semantics (parFlags()returns only the names of TRUE-valued flags, e.g."MyPlot.pearson.filtered";parFlags2()returns every name=value pair regardless of truthiness, e.g."prefix.pearson_TRUE.filtered_TRUE.normalized_FALSE"), and forwarding...through a wrapper also breaksmatch.call()'s ability to recover the original argument names (verified directly: it degrades to"..1","..2", ... inside the callee). So the fix keepsparFlags()'s own logic intact and gives it the same working name-capture lineparFlags2()already uses, preserving its historical behavior instead of silently changing what it returns.Version bumped 1.2.1 → 1.2.2 in
Development/config.RandDESCRIPTION.Validation
Manually confirmed
flag.names_list.all.new(ls = ...)now matchesflag.names_list(ls = ...)exactly, andparFlags(prefix = "MyPlot", pearson, filtered, normalized)now returns"MyPlot.pearson.filtered"as documented (previously always errored). R CMD check's "checking examples" ERROR previously raised byparFlags()'s own broken example is gone. The existingtestthatsuite still passes 7/7.R CMD check now surfaces a different, previously-masked pre-existing bug (
params.2.fname()is missing@exportand has broken internal logic) — out of scope here, fixed in a follow-up PR in this batch. Remaining findings (%!in%Rd-name warning, sandbox locale warning, and doc issues fixed in other independent PRs in this batch) are pre-existing/environment-inherent.Generated by Claude Code