Skip to content

refactor(csv): scan for csv in-tree, dropping vincentlaucsb-csv-parser - #664

Merged
andiwand merged 1 commit into
mainfrom
refactor/csv-in-tree
Aug 8, 2026
Merged

refactor(csv): scan for csv in-tree, dropping vincentlaucsb-csv-parser#664
andiwand merged 1 commit into
mainfrom
refactor/csv-in-tree

Conversation

@andiwand

@andiwand andiwand commented Aug 8, 2026

Copy link
Copy Markdown
Member

🤖 Generated with Claude Code

Split out of #661 — no wasm in here. Stacked on #663.

The vincentlaucsb-csv-parser dependency existed for one function.
csv::check_csv_file answers a single question — does this text parse as a csv
with a consistent field count above one — and CsvFile is a plain
abstract::TextFile with is_decodable() false, so a .csv renders through
the text service either way. Nothing else in the library ever called into the
parser.

It also spawned a thread to do it. CSVReader::begin() and initial_read()
each construct a std::thread and immediately join it, which buys nothing and
cannot be turned off — no compile-time switch, no non-threaded entry point. So
every time open_strategy probed an otherwise unrecognised text file, on every
platform, it spawned and joined a thread for a column count.

The format asked for was the default CSVFormat — comma delimiter, no delimiter
guessing, " quotes, no trim characters — which is plain RFC 4180 and about
forty lines. read_record is exposed alongside check_csv_file because the test
helper needs real field values out of index.csv, and one scanner shared beats a
second one copied into the tests.

Reaching EOF inside a quoted field throws rather than emitting the partial field
as a record. The probe is a classifier, so accepting an unterminated quote widens
it: a,b\n1,"2,3 otherwise reads as two consistent two-field records, and any
prose with an odd number of quotes and an even comma count would have been named
a csv.

Behaviour is unchanged where it is pinned: the corpus the suite enumerates from
index.csv is identical, and csv_file_test keeps its existing assertions and
gains coverage for quoting, crlf, short and long rows, and the unterminated
quote. csv_test.cpp is deleted — it tested the parser, not us.

Full suite green.

@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: 07f6ef134d

ℹ️ 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_util.cpp
@andiwand
andiwand force-pushed the feat/html-no-cache-path branch from d51778f to c485e8f Compare August 8, 2026 18:03
@andiwand
andiwand force-pushed the refactor/csv-in-tree branch 2 times, most recently from cd490a2 to 0e40b6c Compare August 8, 2026 18:09
@andiwand
andiwand force-pushed the feat/html-no-cache-path branch from c485e8f to f0edaed Compare August 8, 2026 18:26
Base automatically changed from feat/html-no-cache-path to main August 8, 2026 18:40
The dependency existed for one function. `csv::check_csv_file` answers a single
question — does this text parse as a csv with a consistent field count above
one — and `CsvFile` is a plain `abstract::TextFile` with `is_decodable()`
false, so a `.csv` renders through the text service either way. Nothing else
in the library ever called into the parser.

It also spawned a thread to do it. `CSVReader::begin()` and `initial_read()`
each construct a `std::thread` and immediately join it, which buys nothing and
cannot be turned off: there is no compile-time switch and no non-threaded
entry point. So every time `open_strategy` probed an otherwise unrecognised
text file, on every platform, it spawned and joined a thread for a column
count.

The format asked for was the default `CSVFormat` — comma delimiter, no
delimiter guessing, `"` quotes, no trim characters — which is plain RFC 4180
and about forty lines. `read_record` is exposed alongside `check_csv_file`
because the test helper needs real field values out of `index.csv`, and one
scanner shared beats a second one copied into the tests.

Reaching EOF inside a quoted field throws rather than emitting the partial
field as a record. The probe is a classifier, so accepting an unterminated
quote widens it: `a,b\n1,"2,3` otherwise reads as two consistent two-field
records, and any prose with an odd number of quotes and an even comma count
would have been named a csv.

Behaviour is unchanged where it is pinned: the corpus the suite enumerates
from `index.csv` is identical, and `csv_file_test` keeps its existing
assertions and gains coverage for quoting, crlf, short and long rows, and the
unterminated quote.
`csv_test.cpp` is deleted — it tested the parser, not us.

Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
Claude-Session: https://claude.ai/code/session_01BjFJ66sma1ye9ZnhM3tNeH
@andiwand
andiwand force-pushed the refactor/csv-in-tree branch from 0e40b6c to c9a9b42 Compare August 8, 2026 18:41
@andiwand
andiwand enabled auto-merge (squash) August 8, 2026 18:45
@andiwand
andiwand merged commit d8c8715 into main Aug 8, 2026
34 checks passed
@andiwand
andiwand deleted the refactor/csv-in-tree branch August 8, 2026 18:51
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