Skip to content

feat(csv): read a csv the way the caller says to - #667

Merged
andiwand merged 1 commit into
mainfrom
feat/csv-options
Aug 9, 2026
Merged

feat(csv): read a csv the way the caller says to#667
andiwand merged 1 commit into
mainfrom
feat/csv-options

Conversation

@andiwand

@andiwand andiwand commented Aug 9, 2026

Copy link
Copy Markdown
Member

🤖 Generated with Claude Code

Third of five stacked PRs. Based on #666 (which is based on #665). Plan: src/odr/internal/csv/PLAN.md.

Detection is a guess. A caller who knows the file should not have to argue with it.

struct CsvOptions final {
  std::optional<TextEncoding> encoding;
  std::optional<char> separator;
  std::optional<char> quote;
};

Unset means detect; set means take as given.

Shaped like decrypt

An immutable handle deriving another handle, narrowed on the concrete type — CsvFile::with_options() next to DocumentFile::decrypt(). That keeps odr::open untouched: no new overload across six open signatures and six DecodedFile constructors, and the bindings already have the call pattern from decrypt (wasm_file.cpp:86, jni_file.cpp:162).

options() returns every field resolved, so a caller can show "detected ;, UTF-8" and offer to change it. That is the flow this exists for: open → see it is wrong → adjust → reopen.

What it deliberately does not copy

The state machine. EncryptionState gates rendering because an encrypted file cannot be rendered at all (file.cpp:173). A csv without options always has a guess, so there is no state where the object cannot be derived. An options_required() would invent a gate the format does not have and force every caller to handle a case that never occurs.

The failure. A wrong password leaves nothing to hand back. A declared separator makes the parser total — a one-column file, prose, an empty file, a truncated quoted field all read as some csv:

CsvFile::from_file(File::from_memory("a\nb\nc\n"), {});                  // throws NoCsvFile
CsvFile::from_file(File::from_memory("a\nb\nc\n"), {.separator = ','});  // fine

One column is no evidence of a csv, and a perfectly good csv once someone says so. What is left to fail is an incoherent dialect — separator equal to the quote, or a line break as a separator — an invalid_argument, a caller mistake rather than bad input. NoCsvFile is now purely a detection failure, which is what its name always claimed.

from_file

Without it, the only handle able to override a verdict would be one you could not obtain — you cannot with_options a file that refused to open. DocumentFile::from_disk/from_memory are the precedent for a static factory.

Not here

  • header_row — would be inert until a sheet exists to mark a header on. Arrives with stage 4, or not at all.
  • Bindings — deliberately deferred. CsvFile is the handle stage 4 hangs document() off, and stage 4 also settles the file category; binding now means binding twice. One pass after the shape settles, tracked as its own stage.

Full suite green (566 unit, 234 reference-output).

@chatgpt-codex-connector chatgpt-codex-connector Bot left a comment

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

💡 Codex Review

Here are some automated review suggestions for this pull request.

Reviewed commit: 1c21594d32

ℹ️ About Codex in GitHub

Codex has been enabled to automatically 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 👍.

When you sign up for Codex through ChatGPT, Codex can also answer questions or update the PR, like "@codex address that feedback".

Comment thread src/odr/internal/csv/csv_file.cpp Outdated
@andiwand
andiwand force-pushed the feat/csv-probe branch 2 times, most recently from 7b05289 to 76b0591 Compare August 9, 2026 10:46
Base automatically changed from feat/csv-probe to main August 9, 2026 10:47
Detection is a guess, and a caller who knows the file should not have to argue
with it. `CsvOptions` carries an encoding, a separator and a quote, each
optional: unset means detect, set means take as given.

The shape follows `decrypt` — an immutable handle deriving another handle,
narrowed on the concrete type — so `odr::open` is untouched, no new overload
across six `open` signatures and six `DecodedFile` constructors, and the
bindings will have the call pattern already. `CsvFile::options()` returns every
field resolved, so a caller can show what was detected and offer to change it,
which is the flow this is for: open, see it is wrong, adjust, reopen.

What it does *not* copy is `decrypt`'s state machine. An encrypted file cannot
be rendered at all until the password arrives, and `capabilities()` masks it
accordingly; a csv without options always has a guess, so there is no state
where the object cannot be derived, and inventing one would force every caller
to handle a case that never occurs.

Nor does the failure map. A wrong password leaves nothing to hand back, whereas
a declared separator makes the parser total: a one-column file, prose, an empty
file and a truncated quoted field all read as some csv. What is left to fail is
an incoherent dialect — a separator equal to the quote, or a line break as a
separator — which is a caller mistake and an `invalid_argument`. `NoCsvFile` is
now purely a detection failure, which is what its name always claimed.

`CsvFile::from_file` is the way in for a file detection would have refused;
without it the only handle able to override a verdict would be one you could
not obtain.

Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
Claude-Session: https://claude.ai/code/session_01QSgWdLTSLCWDeFvbVwZDVU
@andiwand
andiwand merged commit cc406ad into main Aug 9, 2026
36 checks passed
@andiwand
andiwand deleted the feat/csv-options branch August 9, 2026 11:05
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

1 participant