Skip to content

Enable ruff's correctness, complexity and performance rules - #81

Merged
ryan-kipawa merged 12 commits into
masterfrom
fix/ruff-categories
Oct 13, 2026
Merged

ryan-kipawa merged 12 commits into
masterfrom
fix/ruff-categories

Conversation

@ryan-kipawa

@ryan-kipawa ryan-kipawa commented Oct 13, 2026 •

Copy link
Copy Markdown
Collaborator

Refs: no issue; requested directly.

What and why

Ruff tags every rule with a category. This PR selects every stable rule in the correctness, complexity and performance categories and fixes all 56 hits they found on master; none are left as ignores. Ruff has no category selector, so pyproject.toml lists the 163 codes that F, UP, B and E9 did not already select, grouped by linter. Rules for frameworks mikecore does not use are left out: FAST001 (FastAPI) and PD012 (pandas-vet) found nothing and never could, since nothing imports either library. F841 (unused variable) is a correctness rule, so it comes off the ignore list.

The two autofix commits are exact ruff output: rerunning ruff on each parent commit rebuilds them byte for byte (see "Verify it yourself"). Only the other commits need reading.

  • Side finding, fixed: DfsDLLUtil.dfsGetItemValueType stored the native return code in an unused variable (F841), so a failed read returned DataValueType.Instantaneous silently. Every other DfsDLLUtil call checks it with DfsDLL.CheckReturnCode; this one now does too.

Commits

Hand-written commits are bold.

  1. 996deb2 Apply ruff's safe autofixes: ruff check --fix + ruff format output for W605, PLR1730, FURB122, FURB105. No need to read it; it reproduces exactly.
  2. f9e877b Test that reading an item's value type reports a DFS error. Fails on its own.
  3. 475c620 Check the return code when reading an item's value type. The bug fix, two lines.
  4. b687464 Assert the values the reading examples only read. ReadingDfs2File/ReadingDfsuFile read header values and gave the expected value only in a comment. They now assert it, which makes the example tests check something.
  5. b7b8de4 Remove unused variables. The remaining F841 hits, removed with the call kept wherever it has a side effect.
  6. 79a4b46 Apply ruff's unsafe autofixes: ruff check --fix --unsafe-fixes output for TRY201, C408, one SIM102, and the F841 that the bare raise creates. It reproduces exactly; each fix was also read.
  7. 684bd2f Collapse nested ifs that ruff could not rewrite. SIM102, 9 sites. Look at the merged equidistant branches in ReadDfs0DataDouble.
  8. c0a899d Fail tests with self.fail and combine nested with statements. PLR0133, SIM117.
  9. 5581cb5 Note the item value type fix in the changelog.
  10. 594dc32 Enable ruff's correctness, complexity and performance rules. The config. It comes last, so lint passes on every commit.
  11. 3fd9940 Leave out rules for frameworks mikecore does not use. Drops FAST001 and PD012.

Code Review Guide

Glossary

  • Rule category: a field ruff 0.16+ gives each rule in ruff rule --all --output-format json (correctness, suspicious, complexity, performance, pedantic, style, …). The ruff docs filter by it. Config can't select by it.
  • Safe / unsafe fix: ruff applies safe fixes with --fix. Unsafe fixes need --unsafe-fixes because they may change behaviour in edge cases, so the one unsafe-fix commit was also read by hand.
  • Return code (rok): the integer each native DFS function returns. Non-zero means an error, which DfsDLL.CheckReturnCode raises.

Where the change sits

flowchart LR
  A["DfsFile.Open<br/>mikecore/DfsFile.py:785"] --> B["DfsFile.__GetItemInfo<br/>mikecore/DfsFile.py:1504"]
  B --> C["DfsDLLUtil.dfsGetItemValueType<br/>mikecore/DfsFile.py:2114"]
  C --> D["native dfsGetItemValueType"]
  C --> E["DfsDLL.CheckReturnCode<br/>mikecore/DfsDLL.py:336"]
  classDef changed fill:#2da44e,stroke:#1a7f37,color:#fff
  class C changed
Loading

Opening any dfs file reads each item's value type through the changed helper, which now hands the return code to CheckReturnCode. The lint changes touch no other call path.

Conventions followed

Existing codeThis PR
Native return codes go to DfsDLL.CheckReturnCode

mikecore/DfsFile.py:2065-2072

rok = DfsDLL.Wrapper.dfsSetEqTimeAxis(
    headerPointer,
    ...
)
DfsDLL.CheckReturnCode(rok)

mikecore/DfsFile.py:2116-2119

rok = DfsDLL.Wrapper.dfsGetItemValueType(
    itemPointer, ctypes.byref(valueTypeInt)
)
DfsDLL.CheckReturnCode(rok)
Error-reporting tests in test_dfs_basic.py call the DFS wrapper layer directly

tests/test_dfs_basic.py:54-60

def test_error_reporting():
    print(DfsDLL.dfsErrorString(1000))
    try:
        DfsDLL.CheckReturnCode(2007)
    ...

tests/test_dfs_basic.py:63-65

def test_item_value_type_reports_dfs_error():
    with pytest.raises(Exception, match="DFS error code 1000"):
        DfsDLLUtil.dfsGetItemValueType(ctypes.c_void_p())
Fixed bugs get a "Fixed" changelog entry saying what the user now sees

CHANGELOG.md:66-67

- `DfsFile.WriteItemTimeStepNext` reports data shorter than the item with its
  size, instead of raising `AttributeError`.

CHANGELOG.md:68-69

- Reading an item's value type raises the DFS error when the native library
  reports one, instead of returning `DataValueType.Instantaneous`.

Deviations

Nearby codeThis PR
The new test reaches the bug through a null item pointer, not a file. Why: CODING_STANDARDS asks for tests through the public API, but no valid file makes dfsGetItemValueType fail. A null pointer is the one input the real library rejects (it returns rc 1000, with no mocks). Alternative: leave the fix untested.

tests/test_dfs_basic.py:16-19

def test_iteminfo():
    dfs = DfsFileFactory.DfsGenericOpen("testdata/TemporalEqCal.dfs0")
    ...

tests/test_dfs_basic.py:63-65

DfsDLLUtil.dfsGetItemValueType(ctypes.c_void_p())
select lists rule codes instead of linter prefixes. Why: ruff can't select by category, and most linters only partly fall in these three (e.g. Pylint 43 of 95 rules). Alternative: select whole linters, which brings in pedantic and style rules nobody asked for.

pyproject.toml (master):72

select = ["E4", "E7", "E9", "F", "UP", "B"]

pyproject.toml:72-134

select = [
    "E4", "E7", "E9", "F", "UP", "B",
    ...
    # Pylint
    "PLC0131", "PLC0206", ...
The reading examples now assert values that used to sit only in comments. Why: F841 flagged the variables. Deleting them would remove the example's point, while asserting keeps it and makes the test check it. Alternative: delete the lines.

tests/examples_dfs2.py (master):17-18

dx = axisEqD2.Dx  # 900
dy = axisEqD2.Dy  # 900

tests/examples_dfs2.py:17-19

dx = axisEqD2.Dx  # 900
dy = axisEqD2.Dy  # 900
assert (dx, dy) == (900, 900)

Evidence

Before (master with this PR's pyproject.toml, ruff 0.16.10):

$ uvx ruff@0.16.10 check --statistics .
26	F841   	[ ] unused-variable
10	SIM102 	[ ] collapsible-if
 8	W605   	[*] invalid-escape-sequence
 3	PLR1730	[*] if-stmt-min-max
 2	PLR0133	[ ] comparison-of-constant
 2	C408   	[ ] unnecessary-collection-call
 2	TRY201 	[ ] verbose-raise
 1	FURB122	[*] for-loop-writes
 1	SIM117 	[ ] multiple-with-statements
 1	FURB105	[*] print-empty-string
Found 56 errors.

After (this branch):

$ uvx pre-commit run --all-files
ruff check...............................................................Passed
ruff format..............................................................Passed
mixed line ending........................................................Passed
$ uv run pyrefly check
 INFO 0 errors (325 warnings not shown)        # CI ceiling: 330
$ uv run pytest -q
171 passed, 8 xfailed, 8 subtests passed in 5.28s

Autofix commits are pure ruff output. Rerunning ruff on each commit's parent and diffing the trees (diff -r) gives no differences: exit 0 for 996deb2 and exit 0 for 79a4b46.

The side bug. At f9e877b (test only):

E       Failed: DID NOT RAISE Exception
FAILED tests/test_dfs_basic.py::test_item_value_type_reports_dfs_error - Fail...
1 failed, 12 deselected in 0.25s

At 475c620 it passes, and the rest of test_dfs_basic.py passes too (12 passed, 1 xfailed).

The merged ReadDfs0DataDouble branch keeps its output. First three times read from each temporal-axis test file, identical before and after 684bd2f:

TemporalEqCal.dfs0 CalendarEquidistant offset 4.0 times [4.0, 14.0, 24.0]
TemporalEqTime.dfs0 TimeEquidistant offset 3.0 times [3.0, 13.0, 23.0]
TemporalNeqCal.dfs0 CalendarNonEquidistant offset 0.0 times [0.0, 10.0, 20.0]
TemporalNeqTime.dfs0 TimeNonEquidistant offset 3.0 times [3.0, 13.0, 23.0]

Verify it yourself

git fetch origin && git switch fix/ruff-categories && uv sync

# Before: master's code under the new rules -> 56 errors
git restore --source=origin/master -- mikecore miketools tests buildUtil
uvx ruff@0.16.10 check --statistics .
git restore --source=HEAD -- mikecore miketools tests buildUtil

# Each autofix commit is exactly what ruff produces (both diffs are empty)
git switch --detach 996deb2^ && git restore --source=3fd9940 -- pyproject.toml
uvx ruff@0.16.10 check --fix . ; uvx ruff@0.16.10 format .
git diff --stat 996deb2 -- . ':!pyproject.toml'
git restore . && git switch --detach 79a4b46^
uvx ruff@0.16.10 check --fix --unsafe-fixes --select SIM102,C408,TRY201,F841 . ; uvx ruff@0.16.10 format .
git diff --stat 79a4b46
git restore . && git switch fix/ruff-categories

# The side bug: fails on the test commit, passes after the fix
git switch --detach f9e877b && uv run pytest -q tests/test_dfs_basic.py -k item_value_type
git switch fix/ruff-categories && uv run pytest -q tests/test_dfs_basic.py -k item_value_type

# Read only the hand-written changes
git show f9e877b 475c620 b687464 b7b8de4 684bd2f c0a899d 5581cb5 594dc32

Review

  • Suggestion: MapProjection.__del__ assigns _mzMapProjPointer = ctypes.c_void_p() to a local, not self. F841 skips it because the name starts with _. Declined: it was already there, runs only in the finaliser, and no user can observe it, so no public-API test could show it failing. Recorded under "Not in this PR".
  • Question: is leaving out preview rules intended? Answered: yes. Selecting a preview rule needs preview = true, which also changes how stable rules behave across the whole config. The 21 preview rules in these categories (excluding Airflow's) are listed under "Not in this PR". Only TID255 becomes stable in ruff 0.17, so the config comment names it alone.
  • Question: the new test uses a null pointer rather than a file. Answered: no valid file makes the native call fail, and a null pointer is the closest the real library allows. Recorded under Deviations.

One round; verdict CLEAN, no must-fix findings.

Risk

  • Lint config: the new rules only add checks. Lint passes on every commit, because the config lands last. The ruff version is unchanged (CI and pre-commit stay on 0.16.10).
  • Behaviour changes: only dfsGetItemValueType, which now raises where it used to return value type 0. This is safe because every item read from a valid file returns rc 0 there; the full suite opens dfs0/1/2/3/dfsu files and passes (171 passed).
  • Hand-merged conditions (SIM102): each keeps its evaluation order. __FpDynamicIncrement() still runs first on every write, because and evaluates its left side first. The merged ReadDfs0DataDouble branch gives identical output on all four temporal-axis types (Evidence).
  • Example asserts: each Reading* example is called only with the file its comments describe (tests/test_dfs2.py:21, tests/test_dfsu2D.py:18).

Not in this PR

  • 21 preview rules in these categories (Airflow's AIR rules would be left out too): B043 TID255 PLC2801 PLR1712 PLW0244 FURB113 FURB131 FURB142 FURB148 FURB152 FURB156 FURB180 RUF038 RUF050 RUF054 RUF055 RUF056 RUF065 RUF066 RUF072 RUF074. Enabling them needs preview = true for the whole config, which is a separate decision. TID255 also needs CI's ruff bumped to 0.17, which is a workflow change.
  • MapProjection.__del__ local assignment (mikecore/Projections.py:1141): dead but harmless. A fix can't be shown failing by any test a user would see (see Review).

🤖 Generated with Claude Code

ryan-kipawa and others added 10 commits October 13, 2026 09:45
Output of `ruff check --fix` followed by `ruff format`, unedited, for the
rules the next commits enable: W605 (invalid escape sequence), PLR1730
(if-stmt-min-max), FURB122 (for-loop-writes) and FURB105 (print-empty-string).

Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
DfsDLLUtil.dfsGetItemValueType ignores the native return code, so an
invalid item pointer yields value type 0 instead of an error. Fails until
the next commit.

Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
Every other DfsDLLUtil wrapper passes the native return code to
DfsDLL.CheckReturnCode; dfsGetItemValueType stored it in an unused variable
(ruff F841), so a failed read returned value type 0 silently.

Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
ReadingDfs2File and ReadingDfsuFile read header values into variables they
never used (ruff F841), with the expected value only in a comment. Assert
those values so the example tests check them, and drop the unused timeSpan
and ok results in the dfsu examples.

Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
Ruff F841 leftovers: static items DfsuBuilder creates for their side
effect, the result of eumSetupLoadLinux, a calendar flag DfsShowInfo never
reads, and files the builder tests fetch only to trigger GetFile.

Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
Output of `ruff check --fix --unsafe-fixes` for TRY201 (verbose-raise),
C408 (unnecessary dict() call) and the one SIM102 (collapsible-if) ruff
could rewrite, plus its F841 fix for the exception names a bare `raise` no
longer needs. Each was read: `raise` re-raises the same exception and
traceback as `raise e`, `{}` equals `dict()`, and the merged condition in
MapProjection.__del__ short-circuits the same way.

Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
SIM102 (collapsible-if), by hand where ruff offers no fix: each `if A:`
holding only `if B:` becomes `if A and B:`. In ReadDfs0DataDouble the
equidistant time and calendar branches did the same thing, so they become
one condition. Evaluation order is unchanged, so __FpDynamicIncrement
still runs on every write.

Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
PLR0133 (comparison-of-constant): `assert "message" == ""` was a C#-style
Assert.Fail; self.fail says so and keeps the message. SIM117: one `with` for
the subtest and the expected error.

Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
Select every rule ruff 0.16.10 marks stable and tags correctness,
complexity or performance in its rule metadata (`ruff rule --all
--output-format json`, field "category"). F, UP, B and E9 already carried
theirs; the other 165 are listed by code, grouped by linter, because ruff
has no category selector. F841 leaves the ignore list now that the earlier
commits removed every unused variable. TID255 is preview in 0.16.10, so it
waits for a ruff bump.

Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
@ryan-kipawa ryan-kipawa self-assigned this Oct 13, 2026
ryan-kipawa and others added 2 commits October 13, 2026 13:18
FAST001 (FastAPI) and PD012 (pandas-vet) can never fire here, as nothing
imports either library.

Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
@ryan-kipawa
ryan-kipawa marked this pull request as ready for review October 13, 2026 11:22
@ryan-kipawa
ryan-kipawa requested a review from JesperGr as a code owner October 13, 2026 11:22
@ryan-kipawa
ryan-kipawa merged commit 8c0502b into master Oct 13, 2026
10 checks passed
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.

1 participant