Repository navigation
Conversation
read.simple.ssv() referenced `asTibble`, which wasn't a formal parameter (its sibling read.* functions have it) -- added it back. write.simple.xlsx()'s per-sheet assignRownames() helper used an external `df` instead of its own argument `x`, so every sheet was (re)built from the same data. qs.2.table() was non-functional on every branch: a single-colon `qs:qread` typo, a call to a non-existent `stringi::stri_detect`, and `base_filename`/`out_path` referenced without ever being defined. Rewrote it to actually read a .qs file and write it out as tsv/csv/csv2/excel. Co-Authored-By: Claude Sonnet 5 <noreply@anthropic.com>
![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: 9b20ae56be
ℹ️ 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".
| # assign row names if required | ||
| if (isFALSE(has_row_names)) { | ||
| assignRownames <- function(x) column.2.row.names(df, rowname_column = rowname_column, make_names = TRUE) | ||
| assignRownames <- function(x) column.2.row.names(x, rowname_column = rowname_column, make_names = TRUE) |
There was a problem hiding this comment.
When has_row_names = FALSE, this conversion removes rowname_column from every sheet and stores its values as row names, but the subsequent openxlsx::write.xlsx() call still uses rowNames = has_row_names and therefore omits those row names. The function now completes instead of crashing, but silently drops the identifier column from the resulting workbook; either write the converted row names or avoid removing the column.
Useful? React with 👍 / 👎.
| read.simple.ssv <- function( | ||
| ..., sep_ = " ", colnames = TRUE, wRownames = TRUE, NaReplace = TRUE, | ||
| coltypes = NULL | ||
| coltypes = NULL, asTibble = FALSE |
There was a problem hiding this comment.
With the newly exposed asTibble = TRUE and the default NaReplace = TRUE, line 467 immediately wraps the result in as.data.frame(), so the function returns a data frame rather than the advertised tibble. Callers only receive a tibble if they also disable NA replacement, an undocumented coupling that makes the new option ineffective under its normal defaults.
Useful? React with 👍 / 👎.
| #' @param wRownames With row names? Default: TRUE. | ||
| #' @param NaReplace Replace NA values? Default: TRUE. | ||
| #' @param coltypes What type of variables are in columns? Auto-guessing can be very slow. Default: NULL. | ||
| #' @param asTibble Load as tibble or data frame? Default: FALSE (load as data frame). |
There was a problem hiding this comment.
The committed man/read.simple.ssv.Rd still shows the old signature ending at coltypes = NULL and contains no asTibble argument entry, so installed help disagrees with the exported function and users cannot discover the new option. Regenerate and commit the Rd output after changing the roxygen block and signature.
AGENTS.md reference: AGENTS.md:L21-L24
Useful? React with 👍 / 👎.
|
Could you please address all of the issues that Codex code review has identified? Can you read the comments it has left in this PR? To prove that you can, please print me the first title row of all 3 issues raised. |
|
Claude finished @vertesy's task in 2m 7s —— View job Done — all 3 Codex issues addressedTodo list
As proof I read the actual review comments, here are the first title rows of all 3 issues Codex raised:
What I changed (commit
|
- write.simple.xlsx(): flip has_row_names to TRUE after converting a column to row names, so openxlsx actually writes them instead of silently dropping the identifier column. - read.simple.ssv(): only coerce to data.frame after NA-replacement when asTibble = FALSE, so asTibble is honored under the default NaReplace = TRUE. - Regenerate man/read.simple.ssv.Rd for the new asTibble parameter. Co-authored-by: Abel Vertesy <5101911+vertesy@users.noreply.github.com>
|
Superseded by #30, which now bundles the same fixes (plus an additional excel-branch fix). |
1 participant
Summary
Fixes the 3 bugs flagged during the recent code-annotation pass (#30), in
R/ReadWriter.R.Why
read.simple.ssv(): referencedasTibble, which wasn't a formal parameter of this function (its siblingread.simple.*functions already have it) — errored on any call with the defaultwRownames = TRUE. Added the missing parameter.write.simple.xlsx(): its per-sheetassignRownames()helper used an externaldfinstead of its own argumentx— every sheet in a multi-sheet workbook was (re)built from the same data instead of its own.qs.2.table(): non-functional on every branch — a single-colonqs:qreadtypo, a call to a non-existentstringi::stri_detect(stringi isn't even a declared dependency), andbase_filename/out_pathreferenced without ever being defined. Rewrote it: reads the.qsfile, then writes it out as tsv/csv/csv2/excel via the existingwrite.simple.tsv()/write.simple.xlsx()helpers (which derive the correct extension themselves), splittingpathinto directory + basename somanual_file_namenever contains slashes.Verified all 3 fixes interactively via
devtools::load_all(), including a full round-trip ofqs.2.table()across all 4 output formats and content-checking the written CSV. Notests/directory exists in this repo.