chore(tooling): Update static checks by gurukamath · Pull Request #1428 · ethereum/execution-specs

gurukamath · 2025-09-16T10:24:52Z

(closes #1378)
(closes #1397)
(closes #1403 )

What was wrong?

Some of the current static checks are not run on the tests folder. Also, ruff format is not being run.

Related to Issue #1378
Related to Issue #1397
Related to Issue #1403

How was it fixed?

Extend static checks to the tests folder. Only exclude the eest related code

Add ruff format check.

Cute Animal Picture

danceratopz

The updated tox static env looks good, @gurukamath! We just need to make sure that we're running with the appropriate ruff config, to avoid another mass format PR.

A few points on config relevant for ruff format :

To have the ruff format-specific config, as close to the target post-weld config as possible, I'd suggest removing this EELS-specific ignore:

execution-specs/pyproject.toml

Line 483 in 5ddb904

"E203", # Whitespace before ':'
What do you think about removing E501: line-too-long for all of ./tests/ for consistency (my preference: as we're doing this for spec and unit tests in ethereum/execution-spec-tests#2152)? This will not only be a reformat; docstrings, comments will cause trouble.

execution-specs/pyproject.toml

Line 492 in 5ddb904

"E501", # Line too long
We should evaluate the target post-weld docstring rules. Perhaps it's ok to subsequently apply the changes for this ruleset as a separate PR? It will need a little alignment and potentially quite some effort. This is potentially another mass reformat.

Another suggestion, not strictly required in this PR: We will need to update the path(s) here to exclude EEST. So that when we enable EEST ruff commands, these rules aren't applied to tests/eest:

execution-specs/pyproject.toml

Lines 486 to 492 in 5ddb904

    
           [tool.ruff.lint.per-file-ignores] 
        
           "tests/*" = [ 
        
               "D100",         # Missing docstring in public module 
        
               "D101",         # Missing docstring in public class 
        
               "D103",         # Missing docstring in public function 
        
               "D104",         # Missing docstring in public package 
        
               "E501",         # Line too long

danceratopz

Hey @gurukamath, I only reviewed the changes to pyproject.toml and compared them to EEST's ruff config. The [tool.ruff.lint] section looks great; I copied it over to EEST (enabled D400, enabled D415; disabled "A" which will be fixed by Sam's ethereum/execution-spec-tests#2166) and it runs cleanly on our code.

Just one request below to enable D400 🙏

pyproject.toml

codecov-commenter · 2025-09-18T18:53:33Z

⚠️ Please install the to ensure uploads and comments are reliably processed by Codecov.

Codecov Report

✅ All modified and coverable lines are covered by tests.
✅ Project coverage is 86.07%. Comparing base (5ddb904) to head (1da0e96).
❗ Your organization needs to install the Codecov GitHub app to enable full functionality.

Additional details and impacted files

@@             Coverage Diff              @@
##           forks/osaka    #1428   +/-   ##
============================================
  Coverage        86.07%   86.07%           
============================================
  Files              743      743           
  Lines            44072    44072           
  Branches          3891     3891           
============================================
  Hits             37936    37936           
  Misses            5659     5659           
  Partials           477      477

Flag	Coverage Δ
unittests	`86.07% <100.00%> (ø)`

Flags with carried forward coverage won't be shown. Click here to find out more.

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.

🚀 New features to boost your workflow:

❄️ Test Analytics: Detect flaky tests, report on failures, and find test suite problems.
📦 JS Bundle Analysis: Save yourself from yourself by tracking and limiting bundle sizes in JS merges.

danceratopz · 2025-09-19T07:38:50Z

I copied it over to EEST (enabled D400, enabled D415; disabled "A" which will be fixed by Sam's ethereum/execution-spec-tests#2166) and it runs cleanly on our code.

Small correction to the above: EEST will need to enable the other ARG checks to have parity with EELS:

chore(tooling): enable remaining ruff ARG checks execution-spec-tests#2176

gurukamath · 2025-09-19T15:47:06Z

@danceratopz @Carsons-Eels Have updated the PR to stop ignoring D400

danceratopz

Thanks for enabling D400, @gurukamath. At a first glance the changes look good.

Could you just comment whether the changes were fully automated or not? I.e., how much manual resolution did you do? I assume you manually removed or scripted the ^^^ restructuredText underneath headers? What else was required apart from ruff format?

Can you check the comment below? If that change is fine, then I'll do a more thorough review of the changes, based on your answers to the questions (about "how") above.

pyproject.toml

gurukamath · 2025-09-22T10:12:42Z

Thanks for enabling D400, @gurukamath. At a first glance the changes look good.

Could you just comment whether the changes were fully automated or not? I.e., how much manual resolution did you do? I assume you manually removed or scripted the ^^^ restructuredText underneath headers? What else was required apart from ruff format?

Yes. It is a ruff check automated fix + some manual changes. An overwhelming majority of these changes are removing the underline from restructured text. There might be 2-3 slightly more specific cases of manual change. For example like the re-ordering of the restructured test anchor here. Unfortunately, I do not recollect all such cases right now but if you want, I can re-make the commits such that all the automated changes and manual ones are in different commits. Do let me know.

danceratopz

LGTM @gurukamath. Here's what I checked:

I used the ruff settings from this PR in EEST with two exceptions:
i. select: without "ARG"; this is still WIP in EEST,
ii. line-length 99 instead of 79.
I reviewed a random subset of the changes manually: Small comment below.
I asked Claude to review the PR on a per-file basis: All good.

About 2.: Tldr: It's fine from my side, but.. The only comment I have is that the titles in the rendered doc might look a little strange with a period at the end of them. We could potentially modify docc to remove them again? It is kinda crazy to add a period for a linter only to remove them again in the doc flow, BUT, it does allow for consistent lint rules.

The review in 1. was worth it, I found two bugs in EEST's ruff config 😄

We select "C", which doesn't exist. It appears ruff enables both complexity (C90) and flake8-comprehensions (C4) with this -> we should be more explicit and only enable "C4". EELS should add "C4" to its select (27 issues; separate PR)? There are other rules that start with "C", looking into whether these are enabled, or not.
We reverted enabling of "C420" by accident, will fix.

gurukamath · 2025-09-22T13:37:53Z

About 2.: Tldr: It's fine from my side, but.. The only comment I have is that the titles in the rendered doc might look a little strange with a period at the end of them. We could potentially modify docc to remove them again? It is kinda crazy to add a period for a linter only to remove them again in the doc flow, BUT, it does allow for consistent lint rules.

@SamWilsn What do you think of the point made here about docc?

gurukamath · 2025-09-22T13:54:21Z

We select "C", which doesn't exist. It appears ruff enables both complexity (C90) and flake8-comprehensions (C4) with this -> we should be more explicit and only enable "C4". EELS should add "C4" to its select (27 issues; separate PR)? There are other rules that start with "C", looking into whether these are enabled, or not.

Here are my thoughts on the C-space rules. I am ok with ignoring C901 as we had earlier agreed. But I think we should also ignore C401 and C408. The generators and calls which these rules deal with might be a bit of an overhead but they definitely make for more readable code in the spec. So I am in favour of enabling the entire C space and selectively disabling C401, C408 and C901

danceratopz · 2025-09-22T16:24:01Z

Here are my thoughts on the C-space rules. I am ok with ignoring C901 as we had earlier agreed. But I think we should also ignore C401 and C408. The generators and calls which these rules deal with might be a bit of an overhead but they definitely make for more readable code in the spec. So I am in favour of enabling the entire C space and selectively disabling C401, C408 and C901

Thanks for looking at this @gurukamath. This change would be outside the scope of this PR. I haven't looked at how difficult it'd be to able the whole C space, but let's follow up on that elsewhere.

src/ethereum/crypto/blake2.py

src/ethereum/__init__.py

SamWilsn · 2025-09-22T19:28:42Z

src/ethereum/crypto/blake2.py

        """
        (w - R1) value for a given Blake2 flavor.
-        Used in the function G
+        Used in the function G.


Why are we ignoring D205? If we're going to mandate summaries, we should mandate the blank line as well.

I thought I'd check the consequence of enabling D205. This example errors:

""" Generate an index file (index.json) of all the fixtures in the specified directory. """

As the D checks seemingly expect that the summary is on a single line, i.e., it thinks "directory" is the description.

This makes no sense, but it fixes D205:

""" Generate an index file (index.json) of all the fixtures in the specified directory. """

I'm inclined to disable D200, but enable D205. But it's late and will re-visit tomorrow.

Ok, enabling D205 gives 1241 errors. These would not be trivial to fix. We typically launch into multiline docstrings without a summary, so they'd have to re-written. It would result in nicer documentation though.

e.g.

""" A small pytest plugin that shows the a concise help string that only contains the options defined by the plugins defined in execution-spec-tests. """

Funny example, needs fixing anyway 😆

Will need to be handled as part of the markdown conversion. See #671

Agreed, it does need to be fixed but can be rolled into #671

src/ethereum/crypto/blake2.py

SamWilsn · 2025-09-22T19:34:25Z

About 2.: Tldr: It's fine from my side, but.. The only comment I have is that the titles in the rendered doc might look a little strange with a period at the end of them. We could potentially modify docc to remove them again? It is kinda crazy to add a period for a linter only to remove them again in the doc flow, BUT, it does allow for consistent lint rules.

@SamWilsn What do you think of the point made here about docc?

Titles should never have a period, whether it's in the rendered output or the source input. The practical effect of this is that we always need to have a summary line before the first title in a docstring, but I think that actually makes a lot of sense.

gurukamath · 2025-09-23T10:01:42Z

Titles should never have a period, whether it's in the rendered output or the source input. The practical effect of this is that we always need to have a summary line before the first title in a doc-string, but I think that actually makes a lot of sense.

I am going to add this to the issue #671 which is aimed at converting all the doc-strings into markdown. I think we should handle this post weld and enforce the following.

All doc-strings should have the a summary at the start.
The summary should be a single line and should end with a .
There should be a blank line after the summary.

Co-authored-by: Sam Wilson <57262657+SamWilsn@users.noreply.github.com>

Carsons-Eels · 2025-09-24T12:51:38Z

src/ethereum/crypto/blake2.py

        """
        (w - R1) value for a given Blake2 flavor.
-        Used in the function G
+        Used in the function G.


Agreed, it does need to be fixed but can be rolled into #671

Carsons-Eels · 2025-09-24T12:53:29Z

src/ethereum/exceptions.py



-class EthereumException(Exception):     # noqa N818
+class EthereumException(Exception):  # noqa N818


Why do we want it to read EthereumException rather than EthereumError?

gurukamath added 2 commits September 16, 2025 11:52

add ruff format

2cadcba

extend checks to tests

6b56fd2

gurukamath requested review from Carsons-Eels, SamWilsn and danceratopz September 16, 2025 10:27

gurukamath marked this pull request as draft September 18, 2025 08:47

danceratopz reviewed Sep 18, 2025

View reviewed changes

gurukamath added 15 commits September 18, 2025 15:05

remove exclude dirs from ruff config

04dbacf

add D space rules to ruff

98d5485

stop ignoring D200 in ruff

7e120be

stop ignoring D410 in ruff

39dfd3f

stop ignoring D411 in ruff

0af276b

stop ignoring D412 in ruff

aa096ba

stop ignoring D413 in ruff

56650ee

stop ignoring D414 in ruff

6b5af3c

stop ignoring D416 in ruff

7ba0544

stop ignoring E203 in ruff

a24b99f

tests follow the same rules as other code

755b804

tests stop ignoring D103

c5586b9

tests stop ignoring D104

a00af88

tests stop ignoring E501

5014333

clean up ruff config

09c069c

gurukamath marked this pull request as ready for review September 18, 2025 13:33

danceratopz reviewed Sep 18, 2025

View reviewed changes

pyproject.toml Outdated Show resolved Hide resolved

Carsons-Eels mentioned this pull request Sep 19, 2025

chore(tooling): unify eels & eest ruff experience #1404

Closed

6 tasks

stop ignoring D400 in ruff

5c06d96

danceratopz reviewed Sep 22, 2025

View reviewed changes

pyproject.toml Outdated Show resolved Hide resolved

stop ignoring D415

fde14ab

danceratopz approved these changes Sep 22, 2025

View reviewed changes

SamWilsn approved these changes Sep 22, 2025

View reviewed changes

This was referenced Sep 23, 2025

chore: shorten docstrings and single-line comments to 79 (ruff w505: doc-line-too-long) ethereum/execution-spec-tests#2180

Merged

chore(tooling): agree on common post-weld ruff rules #1403

Closed

gurukamath mentioned this pull request Sep 23, 2025

chore(doc): ensure all doc-strings are in markdown #671

Open

gurukamath and others added 3 commits September 23, 2025 12:17

Revert "stop ignoring D200 in ruff"

e5ab72f

enable C4

06b62ec

Apply suggestions from code review

1da0e96

Co-authored-by: Sam Wilson <57262657+SamWilsn@users.noreply.github.com>

gurukamath force-pushed the static-checks branch from be83dae to 1da0e96 Compare September 23, 2025 10:31

Carsons-Eels approved these changes Sep 24, 2025

View reviewed changes

gurukamath merged commit 4ac7a9b into ethereum:forks/osaka Sep 25, 2025
8 checks passed

danceratopz mentioned this pull request Sep 26, 2025

chore(deps,tooling): update ruff rules, bump ruff version ethereum/execution-spec-tests#2210

Merged

5 tasks

	[tool.ruff.lint.per-file-ignores]
	"tests/*" = [
	"D100", # Missing docstring in public module
	"D101", # Missing docstring in public class
	"D103", # Missing docstring in public function
	"D104", # Missing docstring in public package
	"E501", # Line too long



		class EthereumException(Exception): # noqa N818
		class EthereumException(Exception): # noqa N818

Conversation

gurukamath commented Sep 16, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

What was wrong?

How was it fixed?

Cute Animal Picture

Uh oh!

danceratopz left a comment • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

danceratopz left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

codecov-commenter commented Sep 18, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Codecov Report

Uh oh!

danceratopz commented Sep 19, 2025

Uh oh!

gurukamath commented Sep 19, 2025

Uh oh!

danceratopz left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

gurukamath commented Sep 22, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

danceratopz left a comment • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

gurukamath commented Sep 22, 2025

Uh oh!

gurukamath commented Sep 22, 2025

Uh oh!

danceratopz commented Sep 22, 2025

Uh oh!

Uh oh!

Uh oh!

SamWilsn Sep 22, 2025

Choose a reason for hiding this comment

Uh oh!

danceratopz Sep 22, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

danceratopz Sep 22, 2025

Choose a reason for hiding this comment

Uh oh!

danceratopz Sep 22, 2025

Choose a reason for hiding this comment

Uh oh!

gurukamath Sep 23, 2025

Choose a reason for hiding this comment

Uh oh!

Carsons-Eels Sep 24, 2025

Choose a reason for hiding this comment

Uh oh!

Uh oh!

SamWilsn commented Sep 22, 2025

Uh oh!

gurukamath commented Sep 23, 2025

Uh oh!

Carsons-Eels Sep 24, 2025

Choose a reason for hiding this comment

Uh oh!

Carsons-Eels Sep 24, 2025

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

gurukamath commented Sep 16, 2025 •

edited

Loading

danceratopz left a comment •

edited

Loading

codecov-commenter commented Sep 18, 2025 •

edited

Loading

gurukamath commented Sep 22, 2025 •

edited

Loading

danceratopz left a comment •

edited

Loading

danceratopz Sep 22, 2025 •

edited

Loading