Skip to content

Conversation

felix314159
Copy link
Collaborator

@felix314159 felix314159 commented Sep 15, 2025

πŸ—’οΈ Description

Edit: Will take some more time

πŸ”— Related Issues or PRs

N/A.

βœ… Checklist

  • All: Ran fast tox checks to avoid unnecessary CI fails, see also Code Standards and Enabling Pre-commit Checks:
    uvx --with=tox-uv tox -e lint,typecheck,spellcheck,markdownlint
  • All: PR title adheres to the repo standard - it will be used as the squash commit message and should start type(scope):.
  • All: Considered adding an entry to CHANGELOG.md.
  • All: Considered updating the online docs in the ./docs/ directory.
  • All: Set appropriate labels for the changes (only maintainers can apply labels).
  • Tests: Ran mkdocs serve locally and verified the auto-generated docs for new tests in the Test Case Reference are correctly formatted.
  • Tests: For PRs implementing a missed test case, update the post-mortem document to add an entry the list.
  • Ported Tests: All converted JSON/YML tests from ethereum/tests or tests/static have been assigned @ported_from marker.

Copy link
Contributor

@spencer-tb spencer-tb left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM. Thanks for taking this on! I think we can ignore the coverage error for now.

I'd be happy to merge this now. Then for every new EEST PR we try to remember the docstring length. Worst case we come back to it in EELS when we do all the code too!

@spencer-tb
Copy link
Contributor

cc @danceratopz for second opinion on whether to merge now or later

@felix314159 felix314159 changed the title chores(docstring_lengths): shorten all docstrings chores(docstring_lengths): shorten all docstrings (ruff w505: doc-line-too-long) Sep 16, 2025
@danceratopz danceratopz changed the title chores(docstring_lengths): shorten all docstrings (ruff w505: doc-line-too-long) chore: shorten docstrings (ruff w505: doc-line-too-long) Sep 17, 2025
@danceratopz danceratopz added the scope:tooling Scope: Python tools (uv, ruff, tox,...) label Sep 17, 2025
Copy link
Member

@danceratopz danceratopz left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thanks so much for getting on this @felix314159! Looking good so far, but I don't think we're quite finished yet.

First, I noticed a bunch of # noqa: E501 directives applied to docstrings, some of which weren't required anymore, some of which were hiding bad docstrings. This is directly related to this task and it makes sense to get rid as many of these as possible, to stop them proliferating further through the codebase. I did that in 3b91efc

I didn't do any automated reformatting.There are quite a few of them left in tests/unscheduled. If you could remove them and then do your auto-formatting on them that would be amazing πŸ™.

Secondly, during this work, I realized that the command shared in this comment wasn't catching all remaining bad formatting, e.g., this docstring line is still > 79:
https://github.com/felix314159/execution-spec-tests/blob/3b91efc0584e31a7a848ecdd1268c2c30e18e8b1/tests/prague/eip7702_set_code_tx/test_set_code_txs.py#L91

So I had a look and it looks like we need to run this command to verify W505:

uv run ruff check --select W505 --config 'lint.pycodestyle.max-doc-length = 79' src tests .github/scripts

I don't know why this isn't equivalent: uv run ruff check --select W505 --line-length 79 src tests .github/scripts/.

@danceratopz
Copy link
Member

Regarding @spencer-tb's comment:

Then for every new EEST PR we try to remember the docstring length.

I'd even suggest adding this as an additional check in tox's lint environment to make sure we don't have to revisit this:

ruff check --select W505 --config 'lint.pycodestyle.max-doc-length = 79' src tests .github/scripts

And potentially adding a "ruler" at 80 in our VS code config to guide docstring and comment authors, wdyt? We could set [80, 100] here:

"[python]": {
"editor.rulers": [100],

@spencer-tb
Copy link
Contributor

Regarding @spencer-tb's comment:

Then for every new EEST PR we try to remember the docstring length.

I'd even suggest adding this as an additional check in tox's lint environment to make sure we don't have to revisit this:

ruff check --select W505 --config 'lint.pycodestyle.max-doc-length = 79' src tests .github/scripts

And potentially adding a "ruler" at 80 in our VS code config to guide docstring and comment authors, wdyt? We could set [80, 100] here:

"[python]": {
"editor.rulers": [100],

Great idea! Let's add it!

@felix314159
Copy link
Collaborator Author

Thanks so much for getting on this @felix314159! Looking good so far, but I don't think we're quite finished yet.

First, I noticed a bunch of # noqa: E501 directives applied to docstrings, some of which weren't required anymore, some of which were hiding bad docstrings. This is directly related to this task and it makes sense to get rid as many of these as possible, to stop them proliferating further through the codebase. I did that in 3b91efc

I didn't do any automated reformatting.There are quite a few of them left in tests/unscheduled. If you could remove them and then do your auto-formatting on them that would be amazing πŸ™.

Secondly, during this work, I realized that the command shared in this comment wasn't catching all remaining bad formatting, e.g., this docstring line is still > 79: https://github.com/felix314159/execution-spec-tests/blob/3b91efc0584e31a7a848ecdd1268c2c30e18e8b1/tests/prague/eip7702_set_code_tx/test_set_code_txs.py#L91

So I had a look and it looks like we need to run this command to verify W505:

uv run ruff check --select W505 --config 'lint.pycodestyle.max-doc-length = 79' src tests .github/scripts

I don't know why this isn't equivalent: uv run ruff check --select W505 --line-length 79 src tests .github/scripts/.

Interesting, and there does not seem to be a simple fix for docformatter that would be able to fix all of the errors that you can see with that command. We might have to use a different tool / build or own to accomplish this. I still get more than 1100 lines that still need fixing, so we should probably automate this some different way.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
scope:tooling Scope: Python tools (uv, ruff, tox,...) type:chore Type: Chore
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants