Make CSV importer configurable via ConfigArgParse - #166
Conversation
Gerrrr
left a comment
There was a problem hiding this comment.
Thanks for the contribution! Please revert the breaking yaml config change, otherwise LGTM.
| csv_options: | ||
| delimiter: ',' | ||
| quotechar: "'" | ||
| quote_char: "'" |
There was a problem hiding this comment.
Let's keep the existing name and not break existing configs unnecessarily?
There was a problem hiding this comment.
Good catch. I restored quotechar for the existing per-test YAML configuration and added regression coverage for it. The new top-level ConfigArgParse setting remains csv.quote_char, matching --csv-quote-char and CSV_QUOTE_CHAR.
bc0ee05 to
70fdcad
Compare
| ```yaml | ||
| csv: | ||
| delimiter: ';' | ||
| quote_char: "'" |
There was a problem hiding this comment.
You're right, I missed the new top-level YAML example. I've changed it to quotechar too and added regression coverage for the top-level config path.
| BigQueryConfig.NAME, | ||
| ] | ||
|
|
||
| CONFIG_KEY_ALIASES = { |
There was a problem hiding this comment.
I am really on the fence with CONFIG_KEY_ALIASES. @MrlixiangWE can you please elaborate your thoughts on why do we want to introduce them?
My take is, if we really dislike quotechar - now is the time to break backwards compatibility and rename it to quote-char everywhere (although, I'd like to know the justification for it). OTOH if we can live with quotechar, why don't we just add csv-quotechar and call it a day?
There was a problem hiding this comment.
The alias table goes away either way — it was bridging a divergence this PR itself created. But let me give the justification you asked for, which I should have led with in the first round instead of just renaming things.
Across the config surface, multi-word names are separated: time_column, csv_options, update_statement, test_groups, base_branch, bigquery.project_id in YAML; --bigquery-project-id, --since-commit, --config-file on the CLI; BIGQUERY_PROJECT_ID and friends in the environment. quotechar is the one concatenated exception (not counting hostname and username, which are ordinary words). It comes from Python's csv API, but someone writing an otava config shouldn't need to know that — it's just one more special spelling to memorize. That's what the original rename was about — and it matched the names the code itself already used (CsvOptions.quote_char, and the per-test loader key). When you asked to keep the existing name, I took that to mean the per-test files specifically, restored quotechar there, and named the new flag --csv-quote-char to match the other options — then papered over the gap with the alias. In hindsight I should have raised the naming question openly instead of doing it in halves.
One thing worth knowing before we decide: on current master the per-test loader reads only quote_char:
Lines 257 to 259 in cd5bc1e
so the quotechar in docs/CSV.md and examples/csv/config/otava.yaml is silently ignored today — the example never notices because its CSV has no quoted fields. The docs and the code already disagree, and a full rename to quote_char wouldn't break any config that actually works.
So my vote is the deliberate rename: quote_char in YAML, --csv-quote-char, CSV_QUOTE_CHAR, docs and examples updated to match, no aliases and no fallback. If you'd rather keep quotechar, I'll do that instead — --csv-quotechar and done. Either way the PR ends with exactly one public spelling.
Problem
CSV is the only importer whose delimiter and quote character cannot be set through ConfigArgParse. They can currently only be configured per test in YAML.
Before / after
Before: CSV delimiter and quote character can only be configured through per-test
csv_optionsin YAML.After: they can also be set through
--csv-delimiterand--csv-quote-char, the matching environment variables, or acsvsection in the config file. Explicit global values override per-test options; otherwise existing behavior is unchanged.Verification
uv run pytest -q --ignore=tests/graphite_e2e_test.py --ignore=tests/postgres_e2e_test.pyuv run tox -e lintuv build --no-create-gitignoreCloses #119