Dev

[vertesy]

No description provided.

[chatgpt-codex-connector]

💡 Codex Review

Here are some automated review suggestions for this pull request.

Reviewed commit: e43ad13b4b

ℹ️ 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".

[chatgpt-codex-connector]

Declare stringi in DESCRIPTION

Adding this namespace import makes stringi a direct package dependency, but DESCRIPTION still omits it from Imports. Consequently, clean installations do not guarantee that stringi is installed and R CMD check can reject the undeclared namespace dependency; add stringi to DESCRIPTION as required for new dependencies.

AGENTS.md reference: AGENTS.md:L13-L19

Useful? React with 👍 / 👎.

[chatgpt-codex-connector]

Preserve converted row names in the workbook

When has_row_names = FALSE, this now-working branch calls column.2.row.names(), which removes the selected identifier column and stores it as row names. The subsequent openxlsx::write.xlsx(..., rowNames = has_row_names) still passes FALSE, so those row names are also omitted and every identifier in the selected column is silently lost from the workbook; the converted row names need to be enabled for writing on this path.

Useful? React with 👍 / 👎.

[vertesy]

@claude

could you please address the Codex code review suggestions in this PR?

1
"Preserve converted row names in the workbook

When has_row_names = FALSE, this now-working branch calls column.2.row.names(), which removes the selected identifier column and stores it as row names. The subsequent openxlsx::write.xlsx(..., rowNames = has_row_names) still passes FALSE, so those row names are also omitted and every identifier in the selected column is silently lost from the workbook; the converted row names need to be enabled for writing on this path."

I have to say, I I'm not fully confident about this suggestion because these functions have been working very robustly and reliably for years. I'm not sure if simply Codex is misunderstanding something about the operation of these functions, so please be careful when reviewing this comment. Consider the context and the intended output. If you decide to change, then please make it explicitly clear with simple English and examples what and how the output will change.

---

2
"Declare stringi in DESCRIPTION

Adding this namespace import makes stringi a direct package dependency, but DESCRIPTION still omits it from Imports. Consequently, clean installations do not guarantee that stringi is installed and R CMD check can reject the undeclared namespace dependency; add stringi to DESCRIPTION as required for new dependencies."

[claude]

Claude finished @vertesy's task in 3m 16s —— View job

---

Addressed both Codex review suggestions

• Gather context (read files, CLAUDE.md, PR diff)
• Investigate row-names issue in write.simple.xlsx (R/ReadWriter.R:962)
• Fix stringi missing from DESCRIPTION Imports
• Decide on row-names fix carefully (verify against intended behavior)
• Commit & push
• Summarize

1. stringi dependency — fixed, straightforward

R/ReadWriter.R (line ~1150, in read.simple.qs or similar) calls stringi::stri_detect_regex(...), and NAMESPACE already has importFrom(stringi, stri_detect_regex). But DESCRIPTION never listed stringi under Imports, so a clean install wouldn't guarantee it's present and R CMD check would flag it. Added stringi to Imports: in DESCRIPTION.

2. Row names lost in write.simple.xlsx() when has_row_names = FALSE — this one is a real bug, but a very narrow, easy-to-miss one. Here's why it wasn't caught for years, and exactly what changes:

Why you never noticed it: Before this PR, that code branch read column.2.row.names(df, ...) — using a variable df that doesn't exist anywhere in the function. Every time someone actually called write.simple.xlsx(..., has_row_names = FALSE), it would have thrown Error: object 'df' not found. This PR's own fix (changing df → x) is what makes the branch run at all for the first time. So the "robust for years" experience comes from has_row_names = TRUE (the default) — the FALSE path was effectively dead code until this PR revived it.

What the code does today (with just the df→x fix, before my change):

1. You pass a list of data frames with an identifier column (e.g. "gene") and has_row_names = FALSE.
2. column.2.row.names() removes the "gene" column and stores those values as the data frame's row names.
3. The final openxlsx::write.xlsx(..., rowNames = has_row_names) still passes the original has_row_names, which is FALSE — so write.xlsx is told "don't write row names."
4. Net result: the "gene" column is gone from the data (moved to row names in step 2), and then row names aren't written either. The identifiers vanish from the Excel file entirely — not moved to be the first column, just lost.

Example:

df <- data.frame(gene = c("TP53", "MYC"), value = c(1, 2))
write.simple.xlsx(df, has_row_names = FALSE)
# Before my fix: the .xlsx has only one column "value" (1, 2) — "TP53"/"MYC" are gone.
# After my fix:  the .xlsx has row names "TP53"/"MYC" in the leftmost (unlabeled)
#                 column, and "value" next to them — matching what has_row_names = FALSE
#                 is documented to do ("converts the first column to row names").

The fix (R/ReadWriter.R): introduced a separate write_row_names flag that starts equal to has_row_names, but gets set to TRUE right after the conversion happens. has_row_names itself (the input parameter) is untouched, so behavior when it's TRUE (the default, and the path used "for years") is completely unchanged. Only the previously-broken FALSE path now actually preserves the identifiers instead of silently dropping them.

If, after seeing this example, you determine the identifiers were intentionally meant to be dropped in some existing workflow (rather than preserved as row names), let me know and I'll revert just that part — but based on the parameter's own documentation ("converts the first column to row names"), preserving them seems to be the intended behavior.

Note: No R installation was available in this environment, so I could not run R CMD check, devtools::document(), or execute the reproducible example above — please verify locally before merging. git diff confirms only the intended lines changed.

Pushed as commit b74b0d5 to dev.