Dev

[vertesy]

No description provided.

[chatgpt-codex-connector]

💡 Codex Review

Here are some automated review suggestions for this pull request.

Reviewed commit: bbf2f15489

ℹ️ 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]

Preserve the directory of absolute input paths

When path is absolute, base_filename remains absolute and is passed as manual_file_name, while manual_directory is left unset. construct.file.path() therefore selects getwd() as the output directory and combines it with the absolute-looking filename, so conversions can target a malformed cwd-prefixed path (and the reported out_path does not match it). Pass dirname(path) as manual_directory and the extensionless basename(path) as manual_file_name for every output branch.

Useful? React with 👍 / 👎.

[vertesy]

@claude
could you please address:

"Preserve the directory of absolute input paths

When path is absolute, base_filename remains absolute and is passed as manual_file_name, while manual_directory is left unset. construct.file.path() therefore selects getwd() as the output directory and combines it with the absolute-looking filename, so conversions can target a malformed cwd-prefixed path (and the reported out_path does not match it). Pass dirname(path) as manual_directory and the extensionless basename(path) as manual_file_name for every output branch."

---

Also qs.2.table Has some massive edits to its codebase. Could you please double-check it to see if it is indeed correct and does not change the output in an unexpected way? Thank you!

[claude]

Claude finished @vertesy's task in 1m 27s —— View job

---

Claude finished @vertesy's task

Todo list:

• Gather context (read qs.2.table, construct.file.path)
• Fix: preserve directory of absolute input paths in qs.2.table
• Double-check qs.2.table rewrite for correctness/output changes
• Commit & push

1. Fixed: absolute paths in qs.2.table

R/ReadWriter.R:1148 — as Codex flagged, base_filename (derived from path) was passed as manual_file_name with manual_directory left unset. Since construct.file.path() falls back to getwd() when manual_directory is NULL, an absolute path produced a malformed cwd-prefixed output path that didn't match the reported out_path.

Fixed for every output branch (tsv, csv, csv2, excel) by splitting base_filename into:

out_dir  <- dirname(base_filename)
out_name <- basename(base_filename)

and passing manual_directory = out_dir, manual_file_name = out_name to write.simple.tsv() / write.simple.xlsx(). This preserves the directory of absolute paths and is a no-op for relative paths (dirname() returns ".", same as the previous getwd()-based default).

Pushed as d9efb0b.

2. Review of the qs.2.table rewrite

I compared the new version against the one from before this PR (visible in the diff context of the Codex review comment). The old code was actually non-functional — it had several bugs that would have thrown errors or written to the wrong place:

• qs:qread(path) — invalid R syntax (single colon instead of ::), would fail to parse/run.
• Stringendo::ppp(base_filename, out_file) was called with base_filename before it was ever defined, and its result was discarded (not assigned to path_out/out_path).
• The excel branch called ReadWriter::write.simple.xlsx(data, out_path) where out_path was never assigned — would error with "object not found".
• stringi::stri_detect(...) — not a real stringi function (should be stri_detect_regex), so the stopifnot guard would have errored on the very first call.

The rewrite in this PR fixes all of these: it defines base_filename once, correctly builds out_path per format, wraps bare data frames/matrices into a list for write.simple.xlsx() (which expects a list of sheets), and uses the correct stri_detect_regex. So the "massive edit" is a legitimate bug fix rather than a behavioral regression — the previous code could not have produced correct output for any input. With the directory fix above added on top, the function should now work correctly for both relative and absolute .qs paths across all four output formats.

Note: I wasn't able to run the R test suite or devtools::check() in this environment (no Rscript available), so this review is based on static code reading rather than executed tests. It'd be good to smoke-test qs.2.table() with an absolute path locally before merging.
|