Skip to content

♻️ refactor(di): pre-instantiate DI providers and clean up project#79

Merged
itisnotyourenv merged 4 commits into
mainfrom
refactor/di-provider-instantiation
Apr 14, 2026
Merged

♻️ refactor(di): pre-instantiate DI providers and clean up project#79
itisnotyourenv merged 4 commits into
mainfrom
refactor/di-provider-instantiation

Conversation

@itisnotyourenv

Copy link
Copy Markdown
Owner

Pull Request

Description

Centralize Dishka provider instantiation in the DI module instead of duplicating it at each usage site (API app, bot, tests). Also removes outdated plan documents and updates project config.

Type of Change

  • Code quality improvement
  • Configuration change

Changes

DI Provider Refactor

  • infra_providers and interactor_providers are now lists of instances instead of classes
  • Removed duplicated [interactor() for interactor in interactor_providers] pattern from app.py, bot/main.py, and tests/integration/conftest.py
  • Moved I18nProvider() and DBProvider() into infra_providers list
  • Simplified __all__ exports to only expose infra_providers and interactor_providers

Cleanup

  • Deleted 4 outdated plan documents from docs/plans/
  • Updated .gitignore (added *.omc, docs)
  • Updated config-local.yaml test DB credentials
  • Reorganized .claude/settings.local.json permissions

Testing

  • Tests pass locally with my changes
  • New and existing unit tests pass locally with my changes

Code Quality

  • I have performed a self-review of my own code
  • My changes generate no new warnings
  • Pre-commit hooks pass (run pre-commit run --all-files)
  • Ruff linting passes (ruff check src/ tests/)
  • Ruff formatting passes (ruff format src/ tests/ --check)

Additional Notes

The test conftest still instantiates AuthProvider() and DBProvider(worker_postgres_config) directly since it needs a worker-specific postgres config override.


🤖 Generated with Claude Code

Co-Authored-By: Claude Opus 4.6 (1M context) noreply@anthropic.com

Centralize Dishka provider instantiation in the DI module instead of
duplicating it at each usage site (API app, bot, tests). This reduces
boilerplate and ensures consistent provider configuration.

Also removes outdated plan documents, updates .gitignore, and refreshes
local test config.

Co-Authored-By: Claude Opus 4.6 (1M context) <noreply@anthropic.com>
@claude

claude Bot commented Apr 14, 2026

Copy link
Copy Markdown

Code Review: refactor(di) pre-instantiate DI providers

The core goal is solid: eliminating the repeated loop at every call site is a genuine cleanup.

Issues:

1. I18nProvider now included in the API container

infra_providers now includes I18nProvider(), which is also injected into the Litestar API container via *infra_providers. Previously I18nProvider was only wired up in the bot. If I18nProvider reads .ftl files or initialises FluentTranslatorHub at provide-time, this will either silently waste resources on every API startup or raise FileNotFoundError if locale data is not accessible. Worth verifying the API container actually needs i18n, or splitting infra_providers into bot_infra_providers and api_infra_providers if the two entry points need different providers.

2. Shared provider instances across multiple containers

The same list objects (infra_providers, interactor_providers) are passed into make_async_container() in app.py, bot/main.py, and conftest.py. For stateless Dishka providers this is fine, but if any provider ever holds per-container state, sharing instances will cause subtle cross-contamination. Worth a comment so a future provider author does not get surprised.

3. docs added wholesale to .gitignore

Adding docs to .gitignore ignores the entire docs/ directory going forward. Any future documentation committed under docs/ will not be tracked. If docs/ is genuinely no longer used this is fine, but it is an irreversible broad gate.

4. Minor: .gitignore still missing trailing newline.

What is Good:

  • Removes 3 copies of the interactor instantiation loop across app.py, bot/main.py, and conftest.py.
  • conftest.py correctly overrides DBProvider(worker_postgres_config) for worker-specific DBs (documented in PR).
  • .claude/settings.local.json cleanup is much cleaner. The deny list for destructive ops is a good safety baseline.
  • Deleting completed plan docs is appropriate housekeeping.

Questions:

  1. Does I18nProvider initialise locale resources eagerly or lazily? If lazy, the API container concern is moot.
  2. Is there an integration test that creates the API container that would surface a broken I18nProvider setup before production?

itisnotyourenv and others added 3 commits April 14, 2026 15:55
The config-local.yaml was accidentally committed with local credentials
(test_dev_user/test_dev_password/test_dev_db) that don't match the CI
Postgres service (dev_user/dev_password/ask_app), causing all integration
tests to fail with InvalidPasswordError.

Co-Authored-By: Claude Opus 4.6 (1M context) <noreply@anthropic.com>
CI service container credentials (dev_user/dev_password/ask_app) were
out of sync with config-local.yaml (test_dev_user/test_dev_password/
test_dev_db), causing integration tests to fail with
InvalidPasswordError.

Co-Authored-By: Claude Opus 4.6 (1M context) <noreply@anthropic.com>
@claude

claude Bot commented Apr 14, 2026

Copy link
Copy Markdown

Code Review — ♻️ refactor(di): pre-instantiate DI providers and clean up project

Overall Assessment: Request Changes (minor items)

The core refactoring idea is solid — removing the [p() for p in providers] boilerplate at each call site is a clean improvement. A few issues need attention before merging.


🔴 Critical Issues

1. I18nProvider import mismatch in src/infrastructure/di/__init__.py

The new file mixes absolute and relative imports:

from src.infrastructure.i18n import I18nProvider  # absolute
from .auth import AuthProvider                     # relative
from .db import DBProvider                         # relative

Every other provider is imported from a sibling submodule via relative import. I18nProvider should either have its own di/i18n.py provider module (consistent with di/auth.py and di/db.py), or the import should at minimum be relative to stay consistent with the package style. This also exposes an internal implementation detail (infrastructure.i18n) directly from the DI init.

2. docs entry in .gitignore is too broad

docs

This silently ignores any file or directory named docs anywhere in the tree — including any future design documents or ADRs. Use the more targeted pattern:

docs/plans/

🟡 Suggestions

3. Shared provider instances — document the assumption

infra_providers and interactor_providers are now module-level singletons. make_async_container() is called with the same instances in app.py, bot/main.py, and tests. If any Dishka provider acquires state in __init__ (connection pools, locks, etc.) this will be shared across containers in the same process.

Current providers appear stateless in __init__, so this is safe today. A brief comment in di/__init__.py would make the intent explicit and prevent a future __init__ with side-effects from silently breaking isolation:

# Providers are stateless in __init__; instances are shared across containers.
infra_providers = [...]

4. Noisy commit history

The PR contains a commit-revert-re-fix sequence for the credential sync:

  • 🐛 fix(config): revert config-local.yaml DB credentials to match CI
  • Revert "🐛 fix(config): revert config-local.yaml DB credentials..."
  • 🐛 fix(ci): update Postgres credentials to match config-local.yaml

Consider squashing these three commits before merge — the intermediate state is broken (credential mismatch) and adds noise to git log.

5. config-local.yaml committed while listed as gitignored

CLAUDE.md states: "Two config files (both gitignored, copy from config-example.yaml)". Yet config-local.yaml appears in this diff. If it's intentionally tracked (as a template for CI), document that in the PR. If it was committed accidentally, it should be removed from tracking.

6. Write(*lock*) deny rule blocks uv.lock

The new .claude/settings.local.json adds:

"deny": ["Write(*lock*)"]

This will block any agent attempt to update uv.lock — which is a normal operation after adding/updating dependencies. Consider scoping this to secrets patterns (*.key, *.pem) rather than lock files.


✅ What's Good

  • The duplication removal is correct and the diff is clean
  • Moving AuthInteractorProvider into interactor_providers (instead of infra_providers) is semantically more accurate
  • I18nProvider and DBProvider being added to infra_providers means the API and bot now use a single consistent provider list
  • Test conftest correctly keeps its own DBProvider(worker_postgres_config) for worker-isolated DBs — the PR description explains this well
  • .gitignore EOL fix is welcome

Bottom line: Fix the I18nProvider import consistency and the docs gitignore scope. The commit churn is cosmetic but worth addressing before merge.

@itisnotyourenv itisnotyourenv merged commit 9c86fb3 into main Apr 14, 2026
8 of 9 checks passed
@itisnotyourenv itisnotyourenv deleted the refactor/di-provider-instantiation branch April 14, 2026 12:06
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.

1 participant