Skip to content

chore(ci): lint the whole repo, fix pre-existing alembic errors (#352) - #387

Merged
smaramwbc merged 2 commits into
smaramwbc:mainfrom
Vignesh-P-C:fix/352-lint-whole-repo
Sep 6, 2026
Merged

chore(ci): lint the whole repo, fix pre-existing alembic errors (#352)#387
smaramwbc merged 2 commits into
smaramwbc:mainfrom
Vignesh-P-C:fix/352-lint-whole-repo

Conversation

@Vignesh-P-C

@Vignesh-P-C Vignesh-P-C commented Sep 6, 2026

Copy link
Copy Markdown
Contributor

Description

CI only linted server/ and tests/; alembic/ and scripts/ were
never checked and had drifted — running ruff check . finds 2 real
pre-existing errors (an unused import in a migration file, and a
deliberate-but-unflagged E402 in alembic/env.py). CONTRIBUTING.md
tells contributors to run ruff locally without qualifying the path,
so anyone running the obvious ruff check . hits 2 failures on a clean
checkout and has to work out they're pre-existing. This fixes both
errors and widens CI to lint the whole repo.

Related Issue

Closes #352

Type of Change

  • 🐛 Bug fix (non-breaking change that fixes an issue)
  • ✨ New feature (non-breaking change that adds functionality)
  • 💥 Breaking change (fix or feature that would cause existing functionality to change)
  • 📚 Documentation update
  • 🔧 Maintenance (refactoring, dependencies, CI, etc.)
  • 🧪 Test improvement

Changes Made

  • Removed the unused import sqlalchemy as sa in alembic/versions/0014_query_embedding_cache.py
  • Added [tool.ruff.lint.per-file-ignores] in pyproject.toml for alembic/env.py's deliberate late import (must run after the config comment block that resolves sqlalchemy.url), with a comment explaining why
  • Widened the CI lint step from ruff check server/ tests/ to ruff check ., so new top-level directories don't silently go unlinted

Testing

  • Unit tests pass locally
  • Integration tests pass locally
  • Manual testing completed
  • New tests added for new functionality

Test Commands Run

ruff check .   # All checks passed!

Checklist

  • My code follows the project's coding standards
  • I have performed a self-review of my code
  • I have commented my code where necessary
  • I have updated documentation as needed
  • My changes generate no new warnings or errors
  • I have added tests that prove my fix/feature works
  • All existing tests pass
  • I have checked for breaking changes

Screenshots / Recordings

N/A — CI/lint config only. ruff check . output is the proof:
All checks passed!

Additional Notes

Went with ruff check . over explicitly listing scripts//alembic/
alongside server//tests/, per the issue's own reasoning: . stays
correct as new top-level directories get added, where an explicit list
would need updating every time. No test changes needed since this is
lint/CI config only — verification is ruff check . exiting 0, which
it does.

Fixes #352.

@Vignesh-P-C

Copy link
Copy Markdown
Contributor Author

The failing check (CI / test) looks unrelated to this PR — the only
failure in the run is test_subject_entities_uniqueness.py::test_concurrent_upserts_converge_to_one_row,
a concurrency test for the #383 subject_entities uniqueness feature
that landed in main after I branched. This PR only touches an unused
import in a migration file, a ruff per-file-ignore, and one CI YAML
line — nothing related to entity uniqueness or concurrency.

I don't have permission to re-run the job myself, so flagging here in
case it's useful — might be worth a look for #383, or could just be a
one-off race under CI load.

@smaramwbc
smaramwbc merged commit 99317ca into smaramwbc:main Sep 6, 2026
6 checks passed
@smaramwbc

Copy link
Copy Markdown
Owner

Merged — thanks for the clean fix, this closes #352. I verified ruff check . passes repo-wide on the merged tree (including under ruff 0.16.6) and pushed one cosmetic commit restoring the trailing newlines at EOF in the three touched files before merging.

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.

[Feature]: Lint the whole repo in CI - alembic/ and scripts/ are unchecked (2 live ruff errors)

2 participants