Skip to content

Replace the pgclirc ConfigObj parser with configparser - #1635

Open
jackwalkerlabs wants to merge 7 commits into
dbcli:mainfrom
jackwalkerlabs:replace-the-pgclirc-configobj-parser-while-prese
Open

jackwalkerlabs wants to merge 7 commits into
dbcli:mainfrom
jackwalkerlabs:replace-the-pgclirc-configobj-parser-while-prese

Conversation

@jackwalkerlabs

Copy link
Copy Markdown

Closes #1634. This is the separate follow-up requested in the review of #1631.

The main pgclirc reader now uses stdlib configparser, with an adapter for the existing typed settings and named-query persistence interface. The compatibility findings and intentional restrictions are documented in docs/config-compatibility.rst.

The main differences from using configparser directly are:

Existing Pgcli/ConfigObj behavior Handling in this change
Case-sensitive names, literal %, ordinary [DEFAULT] section Preserve them explicitly; disable interpolation and default-section inheritance.
Outer quotes, comma-separated lists and typed accessors Parse existing syntax and retain the accessors used by Pgcli.
Triple-quoted named queries Preserve their exact contents, including blank lines, comment-like lines and indentation.
Comments during saves Update the original file's value spans and retain surrounding comments.
ConfigObj root options and nested [[sections]] Intentionally unsupported, with explicit errors. Single-bracket dotted sections remain supported.

ConfigObj remains a dependency because the PostgreSQL service-file parser still uses it on the current base branch. This PR leaves that separate migration and #1631 unchanged.

Validation: 52 selected configuration/startup/DSN/named-query tests and all 16 additional compatibility characterizations passed in Factory's clean, offline Python 3.11 verification container, with Ruff lint/format checks. The compatibility characterizations were also run successfully against unchanged upstream code before becoming required checks. The full live-PostgreSQL integration suite and other supported Python versions were not run in this environment.

Prepared and verified through FactoryChief, run FC-20260911-5DE277. The run history includes the failed candidates and operator-guided repairs.

@DiegoDAF

Copy link
Copy Markdown
Contributor

I ran this branch against a real-world pgclirc before commenting, and it holds up well. Some data in case it is useful.

Against a 328-line config with 10 sections (including dotted [alias_dsn.init-commands] and space-containing names like [named queries] and [dsn ssh tunnels]), I flattened every key from both parsers and compared: 85 keys each, 84 identical values, zero keys gained or lost. The typed accessors (as_bool, as_int, as_list) all returned the same thing.

The single difference is worth calling out, because it is a fix rather than a regression. ConfigObj turns any comma-containing value into a list. That is fine for destructive_warning, which is read through as_list(), but it also applies to keys the code reads directly. prompt is the one that bites:

prompt = \u@\h, \d>

ConfigObj  ->  ["\\u@\\h", "\\d>"]     a list
this PR    ->  "\\u@\\h, \\d>"         a string

On current main that config crashes at startup with AttributeError: 'list' object has no attribute 'replace' in get_prompt(). This branch makes it work. A comma in a prompt is not exotic, so it may be worth a line in the changelog and a test pinning it, since it is the kind of fix that silently regresses later.

One note from having migrated the other consumer. The doc says ConfigObj stays for service-file parsing; I moved parse_service_info() to configparser in my fork and hit three things that may save you time when that one lands:

  • configparser.ParsingError has no lineno. The line number is in .errors, a list of (lineno, line) pairs. Reading it off the wrong attribute silently reports line 0 for every parse error.
  • The itertools.repeat("\n", skipped_lines) trick is load-bearing for those line numbers: without it, an error on file line 6 is reported as line 3.
  • inline_comment_prefixes defaults to off, which is what you want, but it is worth setting explicitly since it is the whole point of the change and a future reader will wonder.

Happy to review the service-file half when you get to it.

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.

Replace the pgclirc ConfigObj parser while preserving configuration compatibility

2 participants