ci: enforce make lint and fix the three violations it was hiding - #1769
ci: enforce make lint and fix the three violations it was hiding#1769hkad98 wants to merge 1 commit into
Conversation
The job named `lint-and-format-check` only ever ran `make format`, so ruff's linter was never enforced anywhere except pre-commit -- which runs on changed files only. Three violations reached master as a result, and `make lint` has been failing on a clean checkout. Fixes all three, then adds the missing step so the job does what its name says: - sse_client.py `_is_retryable_exc` ended with an `if isinstance(...): return True` / `return False` pair (SIM103). Collapsed to returning the isinstance directly; the comment explaining why RemoteProtocolError is retryable moves above the return and is unchanged. - test_agentic_runner.py imported `AGENTIC_TEST_KINDS` and `AgenticEvalOutcome` inside two test bodies (PLC0415, twice). Both names were already imported at module top -- `AgenticEvalOutcome` identically, `AGENTIC_TEST_KINDS` from a module the file already imports from -- so the local imports were redundant, not deliberate deferrals. Removed, and AGENTIC_TEST_KINDS added to the existing top-level import. No behaviour change: the isinstance chain returns the same value for every input, and the moved imports resolve to the same objects. jira: trivial risk: low
|
No actionable comments were generated in the recent review. 🎉 ℹ️ Recent review info⚙️ Run configurationConfiguration used: Organization UI Review profile: CHILL Plan: Essentials Run ID: 📒 Files selected for processing (3)
Included review availability: 2 reviews are currently available. Your included PR review attempts over the past 7 days set your current allowance at 4 reviews per hour. 📝 WalkthroughWalkthroughThe pull request adds lint execution to the Python test workflow, simplifies SSE retry classification without changing behavior, and moves agentic runner test imports to module scope. ChangesPython quality maintenance
Estimated code review effort: 2 (Simple) | ~10 minutes Merge Risk: ⚪ Minimal · up to This change fixes lint violations and enforces linting in CI without changing product behavior; no actionable merge-blocking risk remains beyond normal checks and review. Suggested reviewers: Poem
🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (4 passed)
Full details: Docstring CoverageExplanation Docstring coverage is 75.00% which is insufficient. The required threshold is 80.00%. Docstring coverage is scoped to functions touched by this diff. Analyzed 4 functions across 2 files. (1 skipped: 1 unsupported.)
✨ Finishing Touches 💡 1📝 Generate docstrings 💡
Comment |
Codecov Report✅ All modified and coverable lines are covered by tests. Additional details and impacted files@@ Coverage Diff @@
## master #1769 +/- ##
==========================================
- Coverage 80.83% 80.82% -0.01%
==========================================
Files 272 272
Lines 19416 19414 -2
==========================================
- Hits 15694 15692 -2
Misses 3722 3722 ☔ View full report in Codecov by Harness. 🚀 New features to boost your workflow:
|
|
Folded into #1768 as a single PR — the lint gate and its three fixes are now commit 3 there. Closing this one; no work is lost. |
1 participant
The job named
lint-and-format-check(rw-python-tests.yaml) only ever ranmake format. Ruff's linter was therefore never enforced in CI — only by pre-commit, which runs on changed files. Three violations reached master as a result, andmake lintfails on a clean checkout of master today:This fixes all three, then adds the missing step so the job does what its name says.
The fixes
SIM103—_is_retryable_excended with anif isinstance(...): return True/return Falsepair. Collapsed to returning theisinstancedirectly. The comment explaining whyRemoteProtocolErroris retryable is preserved verbatim, moved above the return.PLC0415×2 — both function-level imports named symbols already imported at module top:AgenticEvalOutcomeidentically (from gooddata_eval.core.models import AgenticEvalOutcome, DatasetItem), andAGENTIC_TEST_KINDSfromgooddata_eval.cli.agentic_runner, a module the file already imports from. So these were redundant, not deliberate deferrals for a cycle or an expensive import. Removed, withAGENTIC_TEST_KINDSadded to the existing top-level import.No behaviour change: the
isinstancechain returns the same value for every input, and the moved imports resolve to the same objects.Verification
make lintmake formatmake test— gooddata-eval, py314, full suitemake test— gooddata-eval,-k 'agentic_runner or sse'rm -rf .venv && uv sync --only-group lint --locked→make format→make lint)The last row matters: the job installs only the
lintgroup, andmake lintinvokes.venv/bin/ruffdirectly, so no extra dependency is needed for the new step.Note
I deliberately targeted the two tests whose imports moved — the
AGENTIC_TEST_KINDSone is a staleness guard asserting the parametrized case list covers every registered kind, so it would fail loudly if the import change had broken its resolution. It passes.Related: #1768 (uv workspace hardening) is independent — no overlapping files — and among other things makes a bare
uv syncactually install ruff, somake lintworks locally without--all-groups.Summary by CodeRabbit