refactor: replace 1:length(x) with seq_along()/seq_len() sweep - #47
Merged
Conversation
Mechanical, repo-convention sweep: replaced every remaining 1:length(x) idiom (and one 1:(2 * length(x)) variant) with seq_along(x) / seq_len(2L * length(x)) across 13 functions: translate, sortbyitsnames, split_vec_to_list_by_N, zigzagger, colsplit, rowsplit, merge_dfs_by_rn, merge_1col_dfs_by_rn, symdiff.ls, reorder.list, intermingle2lists, as.listalike, imovingSEM, and dput_pretty (two occurrences). This is the same style-convention fix pooled into one PR since it's identical in every location (per the workflow guideline: same code convention everywhere -> one PR). It also happens to fix a genuine edge-case bug: 1:length(x) evaluates to a reversed range c(1, 0) when x has length 0, which previously crashed sortbyitsnames() on empty input with a names-length mismatch. Verified: sortbyitsnames(setNames(numeric(0), character(0))) now returns `named numeric(0)` instead of erroring. Two related, narrower notes: - Left the two 1:(2 * length(df1)) occurrences inside intermingle.cbind() untouched, since open PR #38 (codex/update-intermingle.cbind-function) already fixes those as part of a larger, unrelated hardening change to that function -- no need to duplicate or conflict with it. - Left one occurrence untouched inside a comment string in as.listalike() (line ~3327) that appears to be corrupted/merged with a stray copy of the code below it -- flagging it rather than guessing at a rewrite, per the "don't silently rewrite unclear comments" convention. The actual code line right below it was fixed normally. colsplit()/rowsplit() still fail on a *fully* zero-length factor input (0 unique levels) after this fix, but for a different, deeper reason unrelated to this style sweep: `names(NULL) <- character(0)` itself errors in base R when the accumulator list never gets populated. That's a separate, minor, pre-existing limitation, noted for a future fix rather than folded into this mechanical PR. Verified no regressions: R CMD build + R CMD check, and a full sweep re-running every man/*.Rd example, show the exact same pre-existing, unrelated issues as the unmodified dev branch (already tracked in other PRs in this batch) and nothing new.
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 9c12b7958c
ℹ️ 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.
…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.
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.
Motivation
Per the repo's own convention (integer-literal ranges like
1L:xshould be avoided), this replaces every remaining1:length(x)idiom withseq_along(x)(orseq_len(2L * length(x))for the one doubled-length loop bound) across the file. Pooled into a single PR since it's the exact same mechanical substitution applied identically everywhere — per the workflow guideline, "replacing the same code convention everywhere → one PR."What changed
13 functions, 15 call sites:
translate,sortbyitsnames,split_vec_to_list_by_N,zigzagger,colsplit,rowsplit,merge_dfs_by_rn,merge_1col_dfs_by_rn,symdiff.ls,reorder.list,intermingle2lists,as.listalike,imovingSEM, anddput_pretty(two occurrences).Bonus: also fixes a real edge-case bug
1:length(x)evaluates to the reversed rangec(1, 0)whenxhas length 0 (sincelength(x)is0and1:0counts down, not up). This previously crashedsortbyitsnames()on empty input with a names-length mismatch. Verified:Two things deliberately left alone
intermingle.cbind()'s two1:(2 * length(df1))occurrences — open PR intermingle.cbind: input validation, row-name alignment, safe subsetting, loop fix, docs #38 (codex/update-intermingle.cbind-function) already fixes those as part of a larger, unrelated hardening change to that function (input validation, row-name alignment, safe subsetting). Touching them here would just create a merge conflict with no benefit — left for that PR.as.listalike()(# Iterate over the list, and fill in the elements with the corresponding elements from the vectorfor (v in 1:length(list_wannabe)) {) — this comment looks corrupted, as if merged with a stray copy of the code on the next line. Per the repo's own convention ("if a comment's purpose is unclear or looks outdated, flag it rather than deleting or rewriting it — I'll decide"), I left it as-is and only fixed the actual code line below it. Happy to clean it up if you'd like — just say what it should say.One related, narrower limitation noted (not fixed here)
colsplit()/rowsplit()still fail on a fully zero-length factor input (0 unique levels), but for a different, deeper reason unrelated to this style sweep: when the loop never runs,names(ListOfDFs) <- levelztriesnames(NULL) <- character(0), which itself errors in base R regardless of how the loop bound is written. That's a separate, minor, pre-existing limitation — worth a follow-up if this edge case matters, not folded into this mechanical PR.Testing
R CMD build+R CMD check: no new NOTEs/WARNINGs/ERRORs vs. the unmodifieddevbranch — same pre-existing, unrelated issues only (already tracked in the other PRs in this batch).man/*.Rdexamples individually (tools::Rd2ex+sys.source): identical pass/fail set as unmodifieddev(7 pre-existing failures, all already tracked elsewhere in this batch) — no new failures introduced.translate,zigzagger,dput_pretty,reorder.list,intermingle2lists,imovingSEM, etc.) to confirm unchanged behavior for non-empty input.testthat/automated tests added, per repo convention.Generated by Claude Code