johnsturgeon/tgfp-web
Watch
1
0
Fork
You've already forked tgfp-web
 0
Code Issues 30 Pull requests Projects 1 Releases 41 Packages 1 Wiki Activity Actions

Normalize first-party imports to the app. namespace #372

Merged
johnsturgeon merged 2 commits from normalize-the-import-namespace-371 into main 2026-08-13 20:07:21 +02:00
Conversation 1 Commits 2 Files changed 18 +118 -79
johnsturgeon commented 2026-08-13 19:34:49 +02:00
Owner
Copy link

app/ was a package but also sat on sys.path as a root, so its submodules
were imported under two names in the same process. Verified: db and
app.db resolved to the same file yet were distinct entries in
sys.modules, with distinct module-level state.

The concrete fallout:

  • app/db/init.py builds engine at module scope. Nine modules used
    from db import engine; jobs/sync_team_records.py used
    from app.db import engine -- so that job ran against its own
    SQLAlchemy engine and its own connection pool.
  • Config.get_config() ran twice: two load_dotenv() calls, two full
    os.environ reads, two unrelated Config dataclasses.
  • Two copies of the ESPN client, via the same split.
  • isinstance across the boundary silently returned False.

Both roots were on the path in every context: PYTHONPATH=/ alongside
WORKDIR /app in compose.prod.yml, the script dir plus PyCharm's content
roots locally, and .pylintrc's init-hook injecting ["." , "app"] -- which
is why lint never flagged it.

Standardize on the app.-qualified form. It was already canonical
(alembic/env.py imports app.models, main.py runs "app.main:app") and it
is the only form that survives becoming an installed package in #343.
51 imports across 15 files gain the prefix; intra-package relative
imports are unchanged.

Drop "app" from the pylint init-hook. That is the regression guard as
much as the cleanup: a bare first-party import now fails as import-error
instead of quietly creating a duplicate module.

Regroup imports in six files. Collapsing the namespaces let pylint see
first-party and third-party imports interleaved -- previously the two
halves looked like separate groups.

Resolve static/ and templates/ from file rather than the working
directory, following the existing pattern in routers/mail.py. The app
now boots from anywhere instead of only from inside app/, which #370
depends on.

No Dockerfile or compose changes; PYTHONPATH=/ is now inert and is
removed in #343.

Verified: pylint 10.00/10, flake8 unchanged at 100, no bare first-party
modules in sys.modules, sync_team_records shares app.db.engine, alembic
current still resolves at a793cd810fd0, and the app serves /ping, /login
and nested static assets when launched from the repo root.

Refs #371

Co-Authored-By: Claude Opus 5 noreply@anthropic.com

app/ was a package but also sat on sys.path as a root, so its submodules were imported under two names in the same process. Verified: `db` and `app.db` resolved to the same file yet were distinct entries in sys.modules, with distinct module-level state. The concrete fallout: - app/db/__init__.py builds `engine` at module scope. Nine modules used `from db import engine`; jobs/sync_team_records.py used `from app.db import engine` -- so that job ran against its own SQLAlchemy engine and its own connection pool. - Config.get_config() ran twice: two load_dotenv() calls, two full os.environ reads, two unrelated Config dataclasses. - Two copies of the ESPN client, via the same split. - isinstance across the boundary silently returned False. Both roots were on the path in every context: PYTHONPATH=/ alongside WORKDIR /app in compose.prod.yml, the script dir plus PyCharm's content roots locally, and .pylintrc's init-hook injecting ["." , "app"] -- which is why lint never flagged it. Standardize on the app.-qualified form. It was already canonical (alembic/env.py imports app.models, main.py runs "app.main:app") and it is the only form that survives becoming an installed package in #343. 51 imports across 15 files gain the prefix; intra-package relative imports are unchanged. Drop "app" from the pylint init-hook. That is the regression guard as much as the cleanup: a bare first-party import now fails as import-error instead of quietly creating a duplicate module. Regroup imports in six files. Collapsing the namespaces let pylint see first-party and third-party imports interleaved -- previously the two halves looked like separate groups. Resolve static/ and templates/ from __file__ rather than the working directory, following the existing pattern in routers/mail.py. The app now boots from anywhere instead of only from inside app/, which #370 depends on. No Dockerfile or compose changes; PYTHONPATH=/ is now inert and is removed in #343. Verified: pylint 10.00/10, flake8 unchanged at 100, no bare first-party modules in sys.modules, sync_team_records shares app.db.engine, alembic current still resolves at a793cd810fd0, and the app serves /ping, /login and nested static assets when launched from the repo root. Refs #371 Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
Normalize first-party imports to the app. namespace
All checks were successful
Claude Code Review / claude-review (pull_request) Successful in 2m49s
Details
Python FastAPI Jinja Linting / build (pull_request) Successful in 22s
Details
719e221dc3 
app/ was a package but also sat on sys.path as a root, so its submodules
were imported under two names in the same process. Verified: `db` and
`app.db` resolved to the same file yet were distinct entries in
sys.modules, with distinct module-level state.

The concrete fallout:

- app/db/__init__.py builds `engine` at module scope. Nine modules used
  `from db import engine`; jobs/sync_team_records.py used
  `from app.db import engine` -- so that job ran against its own
  SQLAlchemy engine and its own connection pool.
- Config.get_config() ran twice: two load_dotenv() calls, two full
  os.environ reads, two unrelated Config dataclasses.
- Two copies of the ESPN client, via the same split.
- isinstance across the boundary silently returned False.

Both roots were on the path in every context: PYTHONPATH=/ alongside
WORKDIR /app in compose.prod.yml, the script dir plus PyCharm's content
roots locally, and .pylintrc's init-hook injecting ["." , "app"] -- which
is why lint never flagged it.

Standardize on the app.-qualified form. It was already canonical
(alembic/env.py imports app.models, main.py runs "app.main:app") and it
is the only form that survives becoming an installed package in #343.
51 imports across 15 files gain the prefix; intra-package relative
imports are unchanged.

Drop "app" from the pylint init-hook. That is the regression guard as
much as the cleanup: a bare first-party import now fails as import-error
instead of quietly creating a duplicate module.

Regroup imports in six files. Collapsing the namespaces let pylint see
first-party and third-party imports interleaved -- previously the two
halves looked like separate groups.

Resolve static/ and templates/ from __file__ rather than the working
directory, following the existing pattern in routers/mail.py. The app
now boots from anywhere instead of only from inside app/, which #370
depends on.

No Dockerfile or compose changes; PYTHONPATH=/ is now inert and is
removed in #343.

Verified: pylint 10.00/10, flake8 unchanged at 100, no bare first-party
modules in sys.modules, sync_team_records shares app.db.engine, alembic
current still resolves at a793cd810fd0, and the app serves /ping, /login
and nested static assets when launched from the repo root.

Refs #371

Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
forgejo-actions commented 2026-08-13 19:37:38 +02:00
Copy link

This is a clean, well-scoped mechanical refactor. I inspected the surrounding code to verify the claims in the description hold. Here's my review.

Review: Normalize first-party imports to the app. namespace

This PR converts all first-party imports to the app.-qualified form, drops the "app" entry from pylint's init-hook as a regression guard, and resolves static//templates/ from __file__ instead of the CWD. I verified the change is complete and internally consistent — no functional issues found; the notes below are minor.

Verification I ran:

  • grep for any surviving bare first-party imports (from db, from models, import config, etc.) across all *.py — none remain, matching the "51 imports across 15 files" claim.
  • grep for directory="static"/directory="templates" string literals — none remain; all three template/static call sites (app/main.py:144-145, app/routers/admin.py:19, app/routers/mail.py:38) now derive from __file__.
  • compose.prod.yml mounts ./app:/app with PYTHONPATH: /, so app.main:app and app.models (in alembic/env.py:10) resolve — consistent with the new Dockerfile ENV PYTHONPATH=/.
  • .forgejo/workflows/lint.yml runs pylint $(git ls-files '*.py') from the repo root (CLI infers root, unaffected by the trimmed init-hook), and pytest is stubbed out (echo "pytest is not enabled yet"), so no test job is impacted.
  • entrypoint.sh's exec "$@" correctly hands off to the Dockerfile CMD, and compose.prod.yml sets no command:, so the uvicorn app.main:app ... --no-access-log line is what actually runs — this genuinely fixes the drift called out in the Dockerfile comment (prod was logging every request).

Nits

  • Naming inconsistency for the derived paths: app/main.py:38-39 uses module constants STATIC_FOLDER/TEMPLATE_FOLDER, while app/routers/admin.py:18 and app/routers/mail.py:23 use lowercase template_folder. Harmless, but worth aligning on one convention since they were touched together.
  • The Path(__file__).parent[.parent] / "templates" expression is now duplicated in three modules. Not worth blocking on, but a small shared helper (e.g. in app/config.py) would prevent the three from drifting the way the two uvicorn definitions did.
  • Not introduced by this PR, but adjacent: there is still no Python test suite (pytest is a stub in CI), so this import-boundary change — exactly the kind of thing a smoke test would catch — rests entirely on the manual verification in the description. Worth keeping in mind as #343 lands.

Nothing here blocks. The change is correct, the commit message documents the reasoning well, and the boundary cleanup (single SQLAlchemy engine, single Config) is a real fix rather than cosmetic.

<!-- claude-code-review --> This is a clean, well-scoped mechanical refactor. I inspected the surrounding code to verify the claims in the description hold. Here's my review. --- ## Review: Normalize first-party imports to the `app.` namespace This PR converts all first-party imports to the `app.`-qualified form, drops the `"app"` entry from pylint's `init-hook` as a regression guard, and resolves `static/`/`templates/` from `__file__` instead of the CWD. I verified the change is complete and internally consistent — no functional issues found; the notes below are minor. Verification I ran: - `grep` for any surviving bare first-party imports (`from db`, `from models`, `import config`, etc.) across all `*.py` — **none remain**, matching the "51 imports across 15 files" claim. - `grep` for `directory="static"`/`directory="templates"` string literals — **none remain**; all three template/static call sites (`app/main.py:144-145`, `app/routers/admin.py:19`, `app/routers/mail.py:38`) now derive from `__file__`. - `compose.prod.yml` mounts `./app:/app` with `PYTHONPATH: /`, so `app.main:app` and `app.models` (in `alembic/env.py:10`) resolve — consistent with the new `Dockerfile` `ENV PYTHONPATH=/`. - `.forgejo/workflows/lint.yml` runs `pylint $(git ls-files '*.py')` from the repo root (CLI infers root, unaffected by the trimmed `init-hook`), and pytest is stubbed out (`echo "pytest is not enabled yet"`), so no test job is impacted. - `entrypoint.sh`'s `exec "$@"` correctly hands off to the Dockerfile `CMD`, and `compose.prod.yml` sets no `command:`, so the `uvicorn app.main:app ... --no-access-log` line is what actually runs — this genuinely fixes the drift called out in the Dockerfile comment (prod was logging every request). ### Nits - Naming inconsistency for the derived paths: `app/main.py:38-39` uses module constants `STATIC_FOLDER`/`TEMPLATE_FOLDER`, while `app/routers/admin.py:18` and `app/routers/mail.py:23` use lowercase `template_folder`. Harmless, but worth aligning on one convention since they were touched together. - The `Path(__file__).parent[.parent] / "templates"` expression is now duplicated in three modules. Not worth blocking on, but a small shared helper (e.g. in `app/config.py`) would prevent the three from drifting the way the two uvicorn definitions did. - Not introduced by this PR, but adjacent: there is still no Python test suite (`pytest` is a stub in CI), so this import-boundary change — exactly the kind of thing a smoke test would catch — rests entirely on the manual verification in the description. Worth keeping in mind as #343 lands. Nothing here blocks. The change is correct, the commit message documents the reasoning well, and the boundary cleanup (single SQLAlchemy engine, single `Config`) is a real fix rather than cosmetic.
uv run is not necessary, it's been removed.
All checks were successful
Claude Code Review / claude-review (pull_request) Successful in 1m27s
Details
Python FastAPI Jinja Linting / build (pull_request) Successful in 17s
Details
d4845d68ec 
Also, we hve the docker CMD be the load-bearing app startup, but any bootstrapping / config is in entrypoint.sh
johnsturgeon deleted branch normalize-the-import-namespace-371 2026-08-13 20:07:28 +02:00
Sign in to join this conversation.
Reviewers
No reviewers
Labels
Clear labels   
Kestra

   
bug
Something isn't working

  
enhancement
New feature or request

  
someday
Someday / Maybe

  
subtask

   
☁️ api

   
🎛️ infrastructure

   
🐞 sentry

   
📆 2025 Season
  
📝 pages
allpicks
 Related to the All Picks page

  
📝 pages
picks

  
📝 pages
standings

  
🚀 performance
No labels
Kestra
bug
enhancement
someday
subtask
☁️ api
🎛️ infrastructure
🐞 sentry
📆 2025 Season
📝 pages
allpicks
📝 pages
picks
📝 pages
standings
🚀 performance
Milestone
Clear milestone
No items
No milestone
Projects
Clear projects
No items
No project
Assignees
Clear assignees
No assignees
2 participants
Notifications
Due date
The due date is invalid or out of range. Please use the format "yyyy-mm-dd".

No due date set.

Dependencies

No dependencies set

Reference
johnsturgeon/tgfp-web!372
No description provided.