cal/paper-dynasty-discord
1
0
Fork 0
Code Issues 14 Pull Requests 2 Actions Packages Projects Releases Wiki Activity

fix: tighten ruff.toml + add CI lint step (#108) #109

Merged
cal merged 1 commits from ai/paper-dynasty-discord#108 into main 2026-03-23 12:55:32 +00:00
Conversation 0 Commits 1 Files Changed 2 +64 -7
Claude commented 2026-03-23 06:37:46 +00:00
Collaborator
Copy Link

Closes #108

Summary

  • Removed global F841/F401 suppression from ruff.toml — scoped to legacy directories via per-file-ignores so new files in new paths get full enforcement
  • Added per-file-ignores covering all 26 pre-existing violations (E711/E713/E721/E722/F811/F821) that currently block the pre-commit hook on 12 legacy files
  • Kept global ignores only for genuine project patterns: F403/F405 (star imports in __init__.py), E712 (SQLModel ORM ==), F541 (1000+ legacy f-strings — cosmetic, deferred cleanup)
  • Added .gitea/workflows/ruff-lint.yml — runs ruff check on every PR to main, so violations are caught before merge even if the hook was bypassed with --no-verify

What this fixes

The pre-commit hook was blocked on 26 violations across 12 files with the current ruff.toml. Developers were using --no-verify to bypass. The global F841 suppression also undercut the hook for the exact bug class PRs #37/#38 were created to fix.

After this change:

  • ruff check . returns zero violations (verified locally)
  • The pre-commit hook works cleanly — no --no-verify needed
  • F841 (unused variables) is now enforced on any new file created outside the legacy directories
  • CI enforces ruff on every PR regardless of local hook bypass

Files changed

  • ruff.toml — removed global F841/F401; added [lint.per-file-ignores] section
  • .gitea/workflows/ruff-lint.yml — new CI lint workflow

Other observations

The cogs/players.py:979-1002 has a duplicate paperdex_cardset_slash method definition (F811). The play_lock.py:13,28 has F821 undefined Play (used in a string annotation without import). These are pre-existing issues suppressed per-file here — worth fixing in follow-up issues.

Closes #108 ## Summary - **Removed global F841/F401 suppression** from `ruff.toml` — scoped to legacy directories via `per-file-ignores` so new files in new paths get full enforcement - **Added per-file-ignores** covering all 26 pre-existing violations (E711/E713/E721/E722/F811/F821) that currently block the pre-commit hook on 12 legacy files - **Kept global ignores only for genuine project patterns**: F403/F405 (star imports in `__init__.py`), E712 (SQLModel ORM `==`), F541 (1000+ legacy f-strings — cosmetic, deferred cleanup) - **Added `.gitea/workflows/ruff-lint.yml`** — runs `ruff check` on every PR to `main`, so violations are caught before merge even if the hook was bypassed with `--no-verify` ## What this fixes The pre-commit hook was blocked on 26 violations across 12 files with the current ruff.toml. Developers were using `--no-verify` to bypass. The global F841 suppression also undercut the hook for the exact bug class PRs #37/#38 were created to fix. After this change: - `ruff check .` returns **zero violations** (verified locally) - The pre-commit hook works cleanly — no `--no-verify` needed - F841 (unused variables) is now enforced on any new file created outside the legacy directories - CI enforces ruff on every PR regardless of local hook bypass ## Files changed - `ruff.toml` — removed global F841/F401; added `[lint.per-file-ignores]` section - `.gitea/workflows/ruff-lint.yml` — new CI lint workflow ## Other observations The `cogs/players.py:979-1002` has a duplicate `paperdex_cardset_slash` method definition (F811). The `play_lock.py:13,28` has `F821` undefined `Play` (used in a string annotation without import). These are pre-existing issues suppressed per-file here — worth fixing in follow-up issues.
Claude added 1 commit 2026-03-23 06:37:47 +00:00
fix: tighten ruff.toml + add CI lint step (#108)
All checks were successful
Build Docker Image / build (pull_request) Successful in 1m39s
Details
Ruff Lint / lint (pull_request) Successful in 1m42s
Details
bf7a8f8394 
- Remove global F841/F401 suppression; scope to legacy directories via
  per-file-ignores so new files outside those paths get full enforcement
- Add per-file-ignores covering all 26 pre-existing violations that
  currently block the pre-commit hook (E711/E713/E721/E722/F811/F821)
- Keep global ignores only for genuine project patterns:
  F403/F405 (star imports in __init__.py), E712 (SQLModel ORM ==),
  F541 (1000+ legacy f-strings, cosmetic, deferred cleanup)
- Add .gitea/workflows/ruff-lint.yml — ruff check on every PR to main,
  so violations are caught before merge even if hook was bypassed

Closes #108

Co-Authored-By: Claude Sonnet 4.6 <noreply@anthropic.com>
Claude added the
ai-reviewing
 label 2026-03-23 06:45:52 +00:00
Claude reviewed 2026-03-23 06:47:34 +00:00
Claude left a comment
Author
Collaborator
Copy Link

AI Code Review

Files Reviewed

  • ruff.toml (modified)
  • .gitea/workflows/ruff-lint.yml (added)

Findings

Correctness

  • The migration from global F841/F401 suppression to [lint.per-file-ignores] is the correct ruff pattern for legacy codebases. New files outside the listed directories will now get full enforcement.
  • Ran ruff check . locally against main to baseline violations: violations in cogs/gameplay_legacy.py (E713 ×3), cogs/players.py (F811 ×2), play_lock.py (F821 ×2), tests/**, in_game/game_helpers.py, helpers/main.py, helpers/discord_utils.py — all are covered by the appropriate per-file-ignore entries.
  • discord_ui/** is not listed in per-file-ignores but is also not in the legacy-violation list. Presumably clean per local verification stated in the PR body.
  • The CI workflow structure is correct and mirrors docker-build.yml (Gitea-prefixed action URLs, same checkout/python setup pattern).

Security

  • No security concerns. Config-only change.

Style & Conventions

  • Workflow file follows existing .gitea/workflows/docker-build.yml conventions (ubuntu-latest, actions/checkout@v4, actions/setup-python@v5).
  • Comments in ruff.toml are clear and explain the rationale for each suppression tier.

Suggestions

  • Pin ruff version in CI (pip install ruff==<version>). Unpinned pip install ruff means CI picks up whatever PyPI has at run time. A ruff upgrade could add rules or change behaviour, causing CI failures that aren't reproducible locally. If requirements-dev.txt (added in PR #83) pins ruff, consider pip install -r requirements-dev.txt instead. Not a blocker.
  • PR body correctly flags cogs/players.py:979-1002 (F811 duplicate method) and play_lock.py:13,28 (F821 undefined Play) as pre-existing issues worth follow-up — good housekeeping.

Verdict: APPROVED

Clean, purposeful config change that unblocks the pre-commit hook and adds a meaningful CI safety net. The per-file-ignores approach is the right pattern for incrementally tightening linting on a legacy codebase. Posting as COMMENT — Gitea blocks self-approval.

Automated review by Claude PR Reviewer

## AI Code Review ### Files Reviewed - `ruff.toml` (modified) - `.gitea/workflows/ruff-lint.yml` (added) ### Findings #### Correctness - The migration from global F841/F401 suppression to `[lint.per-file-ignores]` is the correct ruff pattern for legacy codebases. New files outside the listed directories will now get full enforcement. - Ran `ruff check .` locally against `main` to baseline violations: violations in `cogs/gameplay_legacy.py` (E713 ×3), `cogs/players.py` (F811 ×2), `play_lock.py` (F821 ×2), `tests/**`, `in_game/game_helpers.py`, `helpers/main.py`, `helpers/discord_utils.py` — all are covered by the appropriate per-file-ignore entries. - `discord_ui/**` is not listed in `per-file-ignores` but is also not in the legacy-violation list. Presumably clean per local verification stated in the PR body. - The CI workflow structure is correct and mirrors `docker-build.yml` (Gitea-prefixed action URLs, same checkout/python setup pattern). #### Security - No security concerns. Config-only change. #### Style & Conventions - Workflow file follows existing `.gitea/workflows/docker-build.yml` conventions (ubuntu-latest, actions/checkout@v4, actions/setup-python@v5). - Comments in `ruff.toml` are clear and explain the rationale for each suppression tier. #### Suggestions - **Pin ruff version in CI** (`pip install ruff==<version>`). Unpinned `pip install ruff` means CI picks up whatever PyPI has at run time. A ruff upgrade could add rules or change behaviour, causing CI failures that aren't reproducible locally. If `requirements-dev.txt` (added in PR #83) pins ruff, consider `pip install -r requirements-dev.txt` instead. Not a blocker. - PR body correctly flags `cogs/players.py:979-1002` (F811 duplicate method) and `play_lock.py:13,28` (F821 undefined `Play`) as pre-existing issues worth follow-up — good housekeeping. ### Verdict: APPROVED Clean, purposeful config change that unblocks the pre-commit hook and adds a meaningful CI safety net. The per-file-ignores approach is the right pattern for incrementally tightening linting on a legacy codebase. Posting as COMMENT — Gitea blocks self-approval. --- *Automated review by Claude PR Reviewer*
Claude added
ai-reviewed
 and removed
ai-reviewing
 labels 2026-03-23 06:47:54 +00:00
cal approved these changes 2026-03-23 12:50:12 +00:00
cal left a comment
Owner
Copy Link

AI Code Review

Files Reviewed

  • ruff.toml (modified)
  • .gitea/workflows/ruff-lint.yml (added)

Findings

Correctness

  • The per-file-ignores paths (cogs/**, in_game/**, helpers/**, command_logic/**, tests/**, utilities/**, migrations/**) all correspond to directories that exist in the repo. Top-level file suppressions (db_calls_gameplay.py, gauntlets.py, dice.py, manual_pack_distribution.py, play_lock.py, paperdynasty.py, api_calls.py, health_server.py) also all exist.
  • The CI workflow correctly pins to pull_request: branches: [main], matching the existing docker-build.yml trigger pattern.
  • Removed global F841 and F401 suppressions as described — the PR body matches the diff exactly.
  • One note: the workflow installs ruff via a bare pip install ruff without pinning a version. This is a minor consistency concern since requirements-dev.txt likely pins a version. Not a blocker, but a follow-up to pin ruff in CI would prevent drift.

Security

  • No security concerns. The CI job does not use secrets or elevated permissions — it only runs a read-only lint check.

Style & Conventions

  • The new workflow follows the same structure and action URL style (https://github.com/actions/...) as the existing docker-build.yml.
  • Comments in ruff.toml clearly document the rationale for each suppression, which is the right pattern for a config that will need ongoing maintenance.
  • The # Remove entries here as files are cleaned up comment establishes a clear expectation for incremental cleanup.

Suggestions

  • Consider pinning the ruff version in the CI step (pip install ruff==X.Y.Z) to match whatever is in requirements-dev.txt, so local and CI behavior stay consistent.
  • The PR body notes play_lock.py:13,28 has an F821 undefined Play in a string annotation without import — worth filing a follow-up issue to fix that import rather than leaving it indefinitely suppressed.

Verdict: APPROVED

The change does exactly what it claims: removes the two global suppressions that were masking real violations (F841, F401), scopes legacy suppressions to specific directories and files, and adds a CI lint gate so the pre-commit hook bypass path no longer lets violations through. All referenced paths exist. The PR description is accurate and complete. The unversioned pip install ruff is a minor nit that does not block merge.

Automated review by Claude PR Reviewer

## AI Code Review ### Files Reviewed - `ruff.toml` (modified) - `.gitea/workflows/ruff-lint.yml` (added) ### Findings #### Correctness - The `per-file-ignores` paths (`cogs/**`, `in_game/**`, `helpers/**`, `command_logic/**`, `tests/**`, `utilities/**`, `migrations/**`) all correspond to directories that exist in the repo. Top-level file suppressions (`db_calls_gameplay.py`, `gauntlets.py`, `dice.py`, `manual_pack_distribution.py`, `play_lock.py`, `paperdynasty.py`, `api_calls.py`, `health_server.py`) also all exist. - The CI workflow correctly pins to `pull_request: branches: [main]`, matching the existing `docker-build.yml` trigger pattern. - Removed global F841 and F401 suppressions as described — the PR body matches the diff exactly. - One note: the workflow installs `ruff` via a bare `pip install ruff` without pinning a version. This is a minor consistency concern since `requirements-dev.txt` likely pins a version. Not a blocker, but a follow-up to pin ruff in CI would prevent drift. #### Security - No security concerns. The CI job does not use secrets or elevated permissions — it only runs a read-only lint check. #### Style & Conventions - The new workflow follows the same structure and action URL style (`https://github.com/actions/...`) as the existing `docker-build.yml`. - Comments in `ruff.toml` clearly document the rationale for each suppression, which is the right pattern for a config that will need ongoing maintenance. - The `# Remove entries here as files are cleaned up` comment establishes a clear expectation for incremental cleanup. #### Suggestions - Consider pinning the ruff version in the CI step (`pip install ruff==X.Y.Z`) to match whatever is in `requirements-dev.txt`, so local and CI behavior stay consistent. - The PR body notes `play_lock.py:13,28` has an F821 undefined `Play` in a string annotation without import — worth filing a follow-up issue to fix that import rather than leaving it indefinitely suppressed. ### Verdict: APPROVED The change does exactly what it claims: removes the two global suppressions that were masking real violations (F841, F401), scopes legacy suppressions to specific directories and files, and adds a CI lint gate so the pre-commit hook bypass path no longer lets violations through. All referenced paths exist. The PR description is accurate and complete. The unversioned `pip install ruff` is a minor nit that does not block merge. --- *Automated review by Claude PR Reviewer*
cal merged commit e2ddaf75b7 into main 2026-03-23 12:55:32 +00:00
cal deleted branch ai/paper-dynasty-discord#108 2026-03-23 12:55:33 +00:00
Sign in to join this conversation.
Reviewers
No reviewers
cal
Labels
Clear labels   
ai-changes-requested

   
ai-failed

   
ai-pr-opened

   
ai-reviewed

   
ai-reviewing

   
ai-working

   
ai-working

   
bug

   
enhancement

   
feature

New feature proposal

  
in-queue

Approved for development

  
performance

   
security

   
tech-debt

   
todo
No Label
ai-changes-requested
ai-failed
ai-pr-opened
ai-reviewed
ai-reviewing
ai-working
ai-working
bug
enhancement
feature
in-queue
performance
security
tech-debt
todo
Milestone
Clear milestone
No items
No Milestone
Projects
Clear projects
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: cal/paper-dynasty-discord#109
No description provided.
Delete Branch "ai/paper-dynasty-discord#108"

Deleting a branch is permanent. Although the deleted branch may continue to exist for a short time before it actually gets removed, it CANNOT be undone in most cases. Continue?