Skip to content

refactor: extract shared config-file rewrite loop into Config helper#379

Merged
dmccoystephenson merged 1 commit into
mainfrom
refactor/dedupe-config-rewrite-loop
Jun 6, 2026
Merged

refactor: extract shared config-file rewrite loop into Config helper#379
dmccoystephenson merged 1 commit into
mainfrom
refactor/dedupe-config-rewrite-loop

Conversation

@dmccoystephenson
Copy link
Copy Markdown
Member

Summary

DRY refactor consolidating the duplicated config-file rewrite logic into a single source of truth.

  • Config.saveWindowSize and KeyBindings.saveToConfigFile contained a character-for-character identical read → update-matching-keys → append-new-keys → write-back loop, differing only in how savedValues is built and the log message.
  • Extracted the loop into Config._writeKeyValues(self, savedValues, errorMessage) (src/config/config.py). Both callers now build their savedValues dict and delegate; the differing warning text is threaded through errorMessage so existing log behavior is preserved.
  • Removed keyBindings.py's now-orphaned getLogger import and module-level _logger — its only use was the warning that moved into Config.

Net −13 LOC across 3 files; keyBindings.saveToConfigFile shrank from ~42 lines to ~8.

Why

Any future change to the rewrite behavior (comment handling, quoting, encoding, atomic write) previously had to be made in two places and was easy to drift — the classic "bug fixed in one copy but not the other" trap.

Test plan

  • python3 -m compileall src -q — clean
  • SDL_VIDEODRIVER=dummy SDL_AUDIODRIVER=dummy python3 -m pytest — 461 passed (was 460; +1 new)
  • Regression coverage: the existing test_config.py (saveWindowSize) and test_keyBindings.py (saveToConfigFile) suites exercise the extracted helper through both public callers — update-in-place, append, whitespace-before-colon, comment preservation, round-trip — and all still pass unchanged.
  • Added test_write_key_values_updates_appends_and_preserves exercising the helper directly (update + append + comment/blank-line preservation in one call).
  • Formatting run scoped to the 3 changed files only (not tree-wide format.sh), so no unrelated drift entered the diff.

Deferred this cycle (skip reasons)

Closes #366

🤖 Generated with Claude Code

The read/update/append/write-back loop was duplicated verbatim between
Config.saveWindowSize and KeyBindings.saveToConfigFile. Extract it into
Config._writeKeyValues(savedValues, errorMessage); both callers now build
their savedValues dict and delegate. Removes the now-orphaned logger from
keyBindings.py.

Closes #366

Co-Authored-By: Claude Sonnet 4.6 <noreply@anthropic.com>
Copy link
Copy Markdown
Member Author

@dmccoystephenson dmccoystephenson left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Self-review rubric (scored adversarially against the diff; CI is the external anchor):

  • Scope: PASS — 4 files, each necessary for #366 or the required CHANGELOG. The keyBindings.py logger removal is not unrelated churn: _logger/getLogger were orphaned by moving the only warning into Config. Formatting was run scoped to the 3 changed files, so no unrelated drift entered the diff.
  • Tests-new: PASS — the new Config._writeKeyValues helper has a direct test (test_write_key_values_updates_appends_and_preserves, covering update-in-place + append + comment preservation) plus indirect coverage through both public callers.
  • Behavior preservation: PASS — the extracted loop is byte-for-byte the original; saveWindowSize and saveToConfigFile build the same savedValues and pass their original log message through errorMessage. Full suite 461 passed (was 460), including the unchanged saveWindowSize/saveToConfigFile regression suites.
  • Sibling structure: PASS — _writeKeyValues follows the codebase's protected-method convention (leading underscore + camelCase, as in _buildRoomFilePath, _loadOrGenerateRoom); the new test mirrors the existing tmp_path/monkeypatch pattern in test_config.py.
  • Issue resolution: PASS — both named surfaces (config.py:243, keyBindings.py:129) are changed and the loop is now single-sourced.
  • CI: PASS — the test check is green on the PR head.
  • No @property / no new print() / no container import in class files / vendored src/lib untouched: PASS — confirmed via grep over added lines.
  • CHANGELOG updated: PASS — dated [integrated] entry; notes the cycle-2 scoped-formatting lesson was applied.

One judgment call (deferred, not changed here): KeyBindings.saveToConfigFile calls config._writeKeyValues(...), which reaches a single-underscore (protected) method on a different class. This is mildly unidiomatic, but it is the exact API proposed in issue #366 (Config._writeKeyValues with KeyBindings delegating to it), and Config is the natural owner of config-file I/O (it owns getConfigFilePath). A public name (e.g. writeKeyValuesToConfigFile) would avoid the protected-access smell; kept as specced and flagged for reviewer preference.

@dmccoystephenson dmccoystephenson merged commit b0925e6 into main Jun 6, 2026
1 check passed
@dmccoystephenson dmccoystephenson deleted the refactor/dedupe-config-rewrite-loop branch June 6, 2026 23:50
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.

refactor: config-file rewrite loop is duplicated verbatim between config.py and keyBindings.py

1 participant