MrlixiangWE commented on code in PR #166:
URL: https://github.com/apache/otava/pull/166#discussion_r3768410238
##########
otava/config.py:
##########
@@ -128,13 +134,18 @@ class
NestedYAMLConfigFileParser(configargparse.ConfigFileParser):
"""
CLI_CONFIG_SECTIONS = [
+ CsvConfig.NAME,
GraphiteConfig.NAME,
GrafanaConfig.NAME,
SlackConfig.NAME,
PostgresConfig.NAME,
BigQueryConfig.NAME,
]
+ CONFIG_KEY_ALIASES = {
Review 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 namin
g 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`:
https://github.com/apache/otava/blob/cd5bc1eedb2f4ecd851a76ef3211916774cb8afa/otava/test_config.py#L259
so the `quotechar` in
[docs/CSV.md](https://github.com/apache/otava/blob/cd5bc1eedb2f4ecd851a76ef3211916774cb8afa/docs/CSV.md?plain=1#L37)
and
[examples/csv/config/otava.yaml](https://github.com/apache/otava/blob/cd5bc1eedb2f4ecd851a76ef3211916774cb8afa/examples/csv/config/otava.yaml#L27)
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.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: [email protected]
For queries about this service, please contact Infrastructure at:
[email protected]
