refactor: Pythonic & de-overengineering pass 1#33
Merged
Conversation
Contributor
There was a problem hiding this comment.
Pull request overview
This PR applies a set of small refactors across the CLI, web UI, and storage layers to reduce duplication/over-engineering and make locale handling and request parameter wiring more consistent.
Changes:
- Added
helpers.extract_locale()and replaced ad-hoc JSON locale extraction across web UI, diff rendering, and exporters (with new unit tests). - Centralized admin-edit allow-lists via
storage.database.ADMIN_DB_COLUMNS, and simplified web filter parsing using a shared FastAPIDependsdependency. - Updated docs/Make targets and removed a number of internal/experimental design documents from the repo.
Reviewed changes
Copilot reviewed 27 out of 28 changed files in this pull request and generated no comments.
Show a summary per file
| File | Description |
|---|---|
| uv.lock | Updates the editable project version metadata in the lockfile. |
| tests/test_helpers_extract_locale.py | Adds focused unit tests for the new extract_locale helper. |
| kardscm/helpers.py | Introduces extract_locale helper for locale-keyed JSON blobs. |
| kardscm/web/app.py | Uses Depends for shared card-filter parsing; uses extract_locale; centralizes admin form allow-list. |
| kardscm/storage/database.py | Adds ADMIN_DB_COLUMNS constant and uses it in admin update path. |
| kardscm/storage/init.py | Re-exports ADMIN_DB_COLUMNS. |
| kardscm/export/exporters.py | Replaces repeated locale JSON parsing with extract_locale; removes lazy import of abilities. |
| kardscm/diff.py | Uses extract_locale for title/text extraction and removes local JSON parsing helper. |
| kardscm/commands.py | Moves scraping module imports to module-level and uses module imports for baseline helpers. |
| kardscm/scraping/probe.py | Narrows _is_graphql_request exception handling. |
| kardscm/scraping/init.py | Moves LANGUAGE_EN import to module-level (removes lazy import). |
| README.md | Rewrites/reshapes user-facing documentation and usage examples. |
| Makefile | Adds convenience targets (run, sync-diff, web, web-admin). |
| CONTRIBUTING.md | Expands maintainer/developer documentation and updates project structure/flows. |
| AGENTS.md | Simplifies and updates agent operating guidelines. |
| CLAUDE.md | Replaces detailed notes with pointer-style Claude-specific notes. |
| experiments/README.md | Removes experiments workspace readme. |
| docs/api/graphql-schema.md | Removes internal API schema notes doc. |
| docs/superpowers/** | Removes multiple internal planning/spec documents. |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
Promote lazy imports inside baseline_init/baseline_accept (commands.py), translate_card_for_export (exporters.py), and scrape_cards (scraping/__init__.py) to module top. For names that tests patch via mock.patch on the source module (BASELINE_PATH, build_snapshot, save_baseline, fetch_all_cards, build_static_probe), import the containing module rather than the name itself, then access via attribute lookup. Importing the module keeps each call site fresh-resolving against the (potentially patched) module attribute; importing the name snapshots the value at import time and breaks patchability. uvicorn import in web/app.py:run stays lazy with a comment — it is heavy and only needed when actually starting the web server.
except (json.JSONDecodeError, Exception) was redundant — Exception already covers JSONDecodeError. The bare Exception clause also masked unrelated failures. Replace with (json.JSONDecodeError, TypeError) to cover both the documented decode failure and the case where body is unexpectedly non-string.
update_card_admin and _parse_admin_form rebuilt the same
allowed-keys sets on every call, even though the membership is
fully derived from module-level constants.
Add ADMIN_DB_COLUMNS in storage/database.py: the storage-side
allow-list (scalars + ability_* + extra_ability_*). Title/text are
intentionally NOT included — they go through a separate JSON-merge
branch in update_card_admin and including them here would make the
early "if key in allowed" branch silently store plain strings into
locale-keyed JSON columns.
Add _ADMIN_FORM_FIELDS in web/app.py as the form-side composite:
ADMIN_DB_COLUMNS | {"title", "text"}.
Re-export ADMIN_DB_COLUMNS from kardscm.storage for the package API.
Add extract_locale to kardscm.helpers — a single decoder for the "load JSON, look up locale_key, fall back to en-EN or default" idiom that was reimplemented in eight places with subtly different fallback semantics. The helper defaults to en_fallback=False; each call site opts in explicitly. Per-callsite semantics (en-EN fallback, default value) are preserved unchanged: - exporters.py title/text/deck-title/deck-JSON: en_fallback=True with default=str(raw) for titles, "" for text. - diff._title_for_locale: en_fallback=True, default=str(value or ""). - diff._text_for_locale: defaults (en_fallback=False, default=""). - web._decoded_locale_value: defaults — admin form must NOT show English when active locale is missing, otherwise an admin would unknowingly save English into the active locale's JSON key. - web request_save title: default=card_id so the user recognises what changed in their own locale. Drop now-unused diff._parse_json and the json import in diff.py and web/app.py. Add tests/test_helpers_extract_locale.py.
The / and /cards routes had identical 13-parameter Query signatures followed by a call to _read_filters that built CardFilters from those kwargs. FastAPI's Depends solves this natively: a single card_filters_dep dependency function builds the value once, both routes inject it via Depends. Net change: ~30 lines removed, both routes shrink to four parameters. The dataclass CardFilters in web/queries.py is reused unchanged.
Starlette's FormData is not Mapping[str, str] — values may be str or UploadFile. mypy on CI catches the mismatch (locally it slipped through under a more permissive version). Widen the parameter type to Mapping[str, Any]; the parser only reads keys that hold str values and coerces via str() where needed, so behaviour is unchanged.
AABur
force-pushed
the
refactor/pythonic-pass-1
branch
from
553d812 to
7257adc
Compare
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Summary
Five small, independent refactors driven by a Pythonic-style and over-engineering review. Each commit is reviewable on its own; tests stay green after every commit (425 → 439 with the new helper test file).
mock.patch.exceptin probe, drop the redundant bareException.extract_localehelper. Eight callsites collapse, per-callsiteen_fallbackdecision is explicit (admin form decoder must NOT fall back to English).Dependsfor card-filter params — both routes share one dependency builder, ~45 lines removed.Commits
```
553d812 refactor: use FastAPI Depends for card filter params
0f7d2a8 refactor: centralise locale JSON extraction
a280faa refactor: hoist admin allow-lists to module constants
775a035 fix: narrow exception handler in probe
653c7db refactor: move lazy imports to module level
```
Deferred
These were in scope but require modifying existing tests (rule for this cycle: don't touch tests). They are tracked locally in `REFACTORING_PLAN.md` for a follow-up cycle:
Test plan