chore: add ruff linter, format the codebase and gate it in CI - #73
Open
eduardovra wants to merge 9 commits into
Open
chore: add ruff linter, format the codebase and gate it in CI#73eduardovra wants to merge 9 commits into
eduardovra wants to merge 9 commits into
Conversation
Configure ruff (lint + format) at 80 columns, targeting py310. The ruleset mirrors the one used in pagueia, minus TID: pysnes is a single package and relative imports between its modules are the house style. CI runs ruff via uvx, so the lint job skips the project install (PyPy 3.10 + SDL2 headers + Cython) entirely. Refs #34
Mechanical reformat only — no behaviour change. Verified by dumping the AST of all 91 modules before and after: the dumps are byte-identical. Refs #34
Auto-fixable rules applied by `ruff check --fix`; the rest by hand: - cpu.py annotated `hardware_vectors: HardwareVectors` without importing the name — added it under TYPE_CHECKING (F821). - zip() calls now state intent: strict=True where a length mismatch means a corrupt save state, explicit strict=False where the two runs are expected to diverge (B905). - Exceptions raised inside `except` blocks chain with `from e` (B904). - Long comments, docstrings and string literals rewrapped to 80 columns. String splits use adjacent literals, so the parsed AST is unchanged. Deliberate exemptions, each with a reason at the site: Reg.l/Reg.h keep the 65816's low/high byte names (E743), the trace file handles outlive their function (SIM115), and one nested ternary stays an if/else (SIM108). Verified: 388 unit tests, 512 CPU and 512 SPC700 instruction tests pass. Refs #34
The dev group resolved to 0.16.2 while CI pinned 0.15.20, so the two disagreed about formatting. Pin one exact version in both places. 0.16 formats Python blocks inside .md files, which rewrites the hand-tuned examples in AGENTS.md — exclude markdown. Refs #34
The 80-column reformat wrapped three ternaries across multiple lines. A ternary that doesn't fit on one line reads worse than the if/else it replaced, so convert them back — extracting a named variable where the expression sat inside a dict literal or a call argument. Drop SIM108 from the ruleset for the same reason: it pushes if/else blocks into ternaries, and any ternary long enough to exceed 80 columns gets wrapped straight back into a multi-line one. Refs #34
Eleven more sites where the ternary fits on one line but the formatter parenthesized the right-hand side across three. Converted to if/else, except one where the wrap came from a trailing comment — moving the comment above keeps the ternary on a single line. The five identical SPC700 sign extensions collapse to a plain conditional subtraction, which is what they always meant. Verified: 5120 SPC700 instruction tests and 388 unit tests pass. Refs #34
Unreferenced code, confirmed by cross-checking every candidate by hand — vulture can't see the dynamic dispatch this codebase relies on: - Ppu.write_vram: superseded by the $2118/$2119 setters, which each write their own byte so the high byte survives a mode-0 DMA into $2118 alone. - Video.get_renderer_info / toggle_renderer, SDL2Renderer.set_vsync (an empty body) and get_performance_info. Removing the readers left sdl2_calls and renderer_name write-only, so those went too — including an SDL_GetRendererInfo call whose result went nowhere. - data_structures.Tilemap and Tile: never imported anywhere. - PySNES.frame_time, a test helper, a test stub method, and an unused unpacking target. - scripts/trace_matcher.py: the whole file was unreachable, and pysnes.py already compares CPU traces against the bsnes reference. Commented-out code removed, keeping the reasoning as prose where it explained a decision (the emulation-mode direct-page wrap in readDirect, the Program Bank Register living in PC.b). Kept deliberately: Harness.run_until / on_write are public scripting API, and the harness CLI dispatches RPC methods via getattr, so its "unused" methods are live endpoints. Verified: 388 unit, 512 CPU and 2560 SPC700 instruction tests pass. Refs #34
ADC, SBC and LDW each opened with a commented-out `assert False` from when they were unimplemented stubs. All three are complete and covered by the SPC700 instruction tests. Ruff's ERA001 does not flag these, which is why the earlier sweep missed them — it leans on a heuristic that skips short statements. Refs #34
…ents
Two regressions from the 80-column reformat.
Hand-aligned reference tables were exploded one value per line — dsp.py
went from 618 to 1158 lines. The Gaussian interpolation table, the DSP
rate table, the 256-entry SPC700 opcode table and the disassembler
mnemonic tables are restored to their reference layout inside
`# fmt: off` blocks. Two files carry a scoped E501 exemption because
`# fmt: off` stops the formatter but not the line-length rule.
Where a trailing comment was the only reason a statement didn't fit, the
formatter split the code and parked the comment after the closing paren:
self.page_0 = bytearray(
0x0100
) # 0x00-0xFF; upper 16 bytes used by _io_flat mode
Moving the comment above lets the statement sit on one line. Applied to
57 sites across both shapes (comment after the paren, and comment parked
inside it). `# noqa` comments are left alone — suppression is
line-scoped.
Verified: AST identical before and after both passes; 388 unit, 512 CPU
and 5120 SPC700 instruction tests pass.
Refs #34
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.
Adds
ruff(lint + format) at 80 columns, wired into CI and the Makefile.Taking pagueia as the reference, and dropping what doesn't earn its place
here.
What came over, and what didn't
ruff check(pagueia's ruleset, minus TID)mypy— only 405 of 1160 functions carry return annotationsruff formatdjlint— no templatesimport-linter—bus↔cpuare mutually coupled by designmake lint/make formatfail_under— wants a baseline measurement firstTID252(ban relative imports) is deliberately out: it exists in pagueiabecause Django apps are top-level packages, whereas
pysnesis a singlepackage where relative imports are the house style.
CI runs ruff through
uvx, so the lint job skips the project install(PyPy 3.10 + SDL2 headers + Cython) entirely.
Bugs this turned up
cpu.pyannotatedhardware_vectors: HardwareVectorswithout importingthe name (F821).
zip()calls now state intent:strict=Truein save-state loads, where alength mismatch means a corrupt state, and an explicit
strict=Falsewhere the two runs are expected to diverge.
raisestatements insideexceptblocks weren't chaining (from e).Reviewing this
The commits are split so the mechanical churn stays out of the way:
chore:config, make targets, CI workflowstyle:the whole-tree reformat — AST-identical across all 91 modulesfix:the actual lint fixeschore:pin ruff exactly (the dev group had floated to 0.16.2 while CIpinned 0.15.20, so the two disagreed about formatting)
Only commit 3 needs real review.
String splits use adjacent literals, which Python folds at parse time, so
those are AST-identical too; comment and docstring rewraps were checked the
same way. Three exemptions carry a reason at the site:
Reg.l/Reg.hkeepthe 65816's byte-accessor names (E743), the trace file handles outlive their
function (SIM115), and one nested ternary stays an
if/else(SIM108).Verified with 388 unit tests plus 512 CPU and 512 SPC700 instruction tests.
Follow-up
AGENTS.mdstill recommends-n auto --dist=loadgroupfor theSingleStepTests suites. That advice OOMs a 16-core machine (~1.2 GB per
worker) and is tracked separately in #72; left untouched here.
Closes #34