Repository navigation
Apply better-py skill rules - #256
Merged
Merged
Conversation
Fixes the HIGH and MEDIUM findings from the audit of this repo against
the better-py skill (skills/software/better-py in destack):
- ruff.toml: drop the hardcoded target-version, remove TCH, stop
ignoring ANN401, and ban typing.Any, typing.TYPE_CHECKING, getattr,
setattr and hasattr (with an explicit per-file exception for
app/auth/openapi.py, matching the existing architecture-test
allowlist). ty.toml: drop the hardcoded python-version so both tools
read it from requires-python.
- Remove the remaining typing.Any uses: a named OpenAPIDocument alias
in app/auth/openapi.py, pydantic models instead of raw dict/.json()
handling in the entity API tests, the OpenAPI contract test, the
Keycloak setup script and the scenario-test client, and a typed
TokenClaimOverrides/TokenFactory pair for the integration token
helpers instead of **claims: Any.
- Type the behave scenario-test context properly (a runner.pyi stub
with every attribute used) instead of Any, and replace the getattr
fallback in environment.py with context.add_cleanup.
- Remove the one remaining if TYPE_CHECKING: block (app/repository/base.py);
UUID is not a cycle.
- Flatten the four test classes in tests/unit/settings/test_settings.py
into plain functions, per better-py's testing.md.
- Smaller idiom fixes: Self instead of a quoted return type on two
alternate constructors, str.partition instead of str.split, drop a
walrus assignment, make the pagination parameters keyword-only,
name the repeated entity field constraints, use TypeAdapter instead
of cast in the exception tests, and raise OIDCDiscoveryError instead
of a bare RuntimeError from OIDC discovery.
- seed/__main__.py reads DatabaseSettings instead of os.environ
directly, and main() now returns an int with raise SystemExit(main()),
matching app/main.py and the skill's script.md.
- The OpenAPI snapshot update flag is now a pytest --update-openapi-snapshot
option instead of an env var read inside the test.
- Makefile: lint and format now also cover .docker/.
Deliberately left unchanged, with the reason:
- alembic/env.py's os.environ.get("DATABASE_URL") fallback: switching
it to DatabaseSettings would make the existing
try/except ModuleNotFoundError fallback (which lets alembic run
before "app" is importable) inconsistent, and I can't run
`make test-integration` here (no Docker) to verify a change to a
migration entry point.
- .docker/configure_keycloak_client.py's remaining os.environ reads:
it configures Keycloak, not the application, so it doesn't belong
behind app.settings.
- tests/integration/conftest.py's INTEGRATION_TEST_CONTAINER_PASSWORD
read: a test-only container password override, not app config.
- Docker Compose services already run python in exec form as PID 1
(no shell wrapper), so SIGTERM is delivered without an init process.
- Several JUDGEMENT-level findings from the audit (mixins on the
SQLAlchemy models, the FastAPI subclass, package-by-layer under
import-linter, session-scoped fixtures, generality the tests alone
exercise) are architecture choices this template makes on purpose,
not violations.
Verified: make check (ruff, ruff format, ty --error-on-warning,
import-linter, 167 unit tests), make lint-typos. make test-integration
was not run (needs Docker, unavailable in this environment); it is
required by CI and by this repo's own PR template for database-layer
changes, so CI will need to confirm the seed/__main__.py and
tests/integration/conftest.py and tests/integration/test_entities_api.py
changes.
🤖 Generated with Claude Code
Co-Authored-By: Claude <noreply@anthropic.com>
Codex Review SummaryThis comment shows the latest Codex review activity on this pull request.
ℹ️ About Codex in GitHubYour team has set up Codex to review pull requests in this repo. Reviews are triggered when you
Codex reacts with 👀 while any review is running, comments if it has suggestions, and reacts with 👍 once all reviews finish with no findings. |
SonarCloud flagged this function's complexity at 19 against the 15 allowed, counted as new code because this PR changed its annotation line. Extract the per-operation and description-merge logic into two small functions so the loop body is a single call. Verified: make check, pytest tests/unit/test_openapi_contract.py. 🤖 Generated with Claude Code Co-Authored-By: Claude <noreply@anthropic.com>
Owner
Author
|
@codex review |
|
Codex Review: Didn't find any major issues. More of your lovely PRs please. Reviewed commit: ℹ️ About Codex in GitHubYour team has set up Codex to review pull requests in this repo. Reviews are triggered when you
If Codex has suggestions, it will comment; otherwise it will react with 👍. Codex can also answer questions or update the PR. Try commenting "@codex address that feedback". |
Code review fixes: - seed/__main__.py imported app.settings.DatabaseSettings, but the migration image copies only alembic/ and seed/ and installs only the migration group, so `docker compose --profile seed run seed` would fail with ModuleNotFoundError. CI never runs the seed profile, so it would not have caught this. Read DATABASE_URL directly again; the int return and raise SystemExit(main()) stay. - tests/integration/test_entities_api.py declared a ProblemBody model that duplicated app.exceptions.ProblemDetails field for field. Validate responses with ProblemDetails instead. Verified: make check.
Owner
Author
|
@codex review |
|
|
Codex Review: Didn't find any major issues. Swish! Reviewed commit: ℹ️ About Codex in GitHubYour team has set up Codex to review pull requests in this repo. Reviews are triggered when you
If Codex has suggestions, it will comment; otherwise it will react with 👍. Codex can also answer questions or update the PR. Try commenting "@codex address that feedback". |
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
1 participant
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.
What changes
Applies the upgraded
better-pyskill to this repo and fixes the HIGH and MEDIUM findings from an audit of this repo against that skill (the skill lives atskills/software/better-pyin thedestackrepo; the full audit is linked below).Main changes:
ruff.toml/ty.tomlno longer hardcode the Python version (read fromrequires-python), dropTCH, stop ignoringANN401, and bantyping.Any,typing.TYPE_CHECKING,getattr,setattrandhasattr— with the existingapp/auth/openapi.pyexception kept as an explicit per-file ignore.typing.Any: a namedOpenAPIDocumentalias, pydantic models in the entity API and OpenAPI contract tests and the Keycloak/scenario-test clients, and a typedTokenClaimOverrides/TokenFactorypair for the integration token helpers.contextis now typed via arunner.pyistub instead ofAny+getattr.if TYPE_CHECKING:block (UUIDis not a cycle).tests/unit/settings/test_settings.pyinto plain functions.Selfinstead of a quoted return type,str.partitioninstead ofstr.split, dropped a walrus assignment, keyword-only pagination parameters, named entity field constraints,TypeAdapterinstead ofcast, a domainOIDCDiscoveryErrorinstead of a bareRuntimeError.seed/__main__.pyandapp/main.pyreturn an exit code throughraise SystemExit(main()).--update-openapi-snapshotoption instead of an env var read inside the test (make openapi-snapshotupdated to match).make lint/make formatnow also cover.docker/.Deliberately left unchanged, with the reason in the commit messages:
seed/__main__.py's andalembic/env.py'sos.environreads (both run in the migration image, which contains neitherapp/nor pydantic-settings) (coupled to its existing import-guard pattern, and I have no Docker here to verify a change to a migration entry point), the Keycloak setup script's remaining env reads (it configures Keycloak, not this app), the integration tests' container-password override (test-only), and the no-init-process audit note (Compose already runspythonin exec form as PID 1, so signals are delivered without one). A handful of JUDGEMENT-level audit findings (SQLAlchemy mixins, theFastAPIsubclass, package-by-layer under import-linter) are architecture choices this template makes on purpose and are not changed.Audit this PR is based on: https://manuscript.subhransu.de/3xt6abhd-fastapi-template-vs-better-py-audit-of-where-the-project-dif
Gates
make checkis greenmake test-integrationis green (required for model, repository, database or migration changes) — not run locally, no Docker available in this environment. This PR touches the integration test helpers and fixtures, so please confirm CI'stest-integrationjob is green before merging.tests/contract/openapi.jsonis updated and the diff is intended — no API change, snapshot untouched..docker/— no new roles.Notes for the reviewer
make test-integrationcould not be run locally (Docker is unavailable in this environment). I'll be watching CI and will act on anything it finds.__init__.pyfiles, test helpers reading private attributes, etc.) to keep this change reviewable; happy to follow up separately if wanted.🤖 Generated with Claude Code. Written by claude-sonnet-5, reviewed and corrected by claude-opus-5-5.