Make CSV importer configurable via ConfigArgParse - #166
Conversation
Gerrrr
left a comment
Gerrrr
left a comment
There was a problem hiding this comment.
Thanks for the contribution! Please revert the breaking yaml config change, otherwise LGTM.
bc0ee05 to
70fdcad
Compare
6b525db to
065b418
Compare
There was a problem hiding this comment.
Pull request overview
Adds global CSV delimiter and quote-character configuration through CLI, environment variables, and YAML.
Changes:
- Introduces
CsvConfigand applies global overrides to CSV tests. - Adds parser, precedence, and help-output tests.
- Updates CSV examples and documentation.
Reviewed changes
Copilot reviewed 11 out of 11 changed files in this pull request and generated 1 comment.
Show a summary per file
| File | Description |
|---|---|
otava/csv_options.py |
Defines global CSV parser options. |
otava/config.py |
Loads and propagates global CSV configuration. |
otava/test_config.py |
Applies global overrides to per-test CSV options. |
tests/config_test.py |
Tests parsing, precedence, and propagation. |
tests/cli_help_test.py |
Updates expected CLI help. |
tests/cli_options_test.py |
Corrects the per-test quote key. |
tests/csv_e2e_test.py |
Corrects the CSV E2E configuration. |
tests/resources/sample_config.yaml |
Adds per-test CSV options. |
tests/resources/substitution_test_config.yaml |
Adds global CSV substitution values. |
examples/csv/config/otava.yaml |
Corrects the example quote key. |
docs/CSV.md |
Documents global CSV configuration. |
Suppressed comments (1)
otava/csv_options.py:41
csv.readeralso requiresquotecharto be exactly one character, while this option permits empty or multi-character values from the CLI, environment, or YAML. Those values fail only when a CSV test runs. Reuse the delimiter's parse-time character validator here so configuration errors are reported immediately.
"--csv-quote-char",
💡 Add a code-review agent skill or configure MCP servers for context-aware, tailored reviews. Learn more in the docs.
| @staticmethod | ||
| def add_parser_args(arg_group): | ||
| arg_group.add_argument( | ||
| "--csv-delimiter", |
There was a problem hiding this comment.
Good feedback. I will merge this PR after this is fixed.
There was a problem hiding this comment.
Good catch — fixed. Both options now use a single_character type validator, so a multi-character value is rejected when the configuration is parsed:
error: argument --csv-delimiter: must be a single character, got '::'
That covers the config file, the CLI flag and the environment variable, with tests for each. The per-test csv_options keys still accept multi-character values, but that behavior predates this PR — I can tighten it here too if you prefer.
Also rebased onto de46ddf.
CSV delimiter and quote character could only be set per test in YAML. They can now also be set through a top-level csv section in the config file, the --csv-delimiter and --csv-quote-char options, and the CSV_DELIMITER and CSV_QUOTE_CHAR environment variables. Explicit global values override per-test csv_options; otherwise existing behavior is unchanged. The per-test YAML key is renamed from quotechar to quote_char. The loader only ever read quote_char, so the documented quotechar spelling was silently ignored and no working configuration depends on it. Closes apache#119
065b418 to
573e54f
Compare
Gerrrr
left a comment
Gerrrr
left a comment
There was a problem hiding this comment.
LGTM! Thank you for this contribution and for addressing feedback!
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