Conversation
bbd93fe to
0bf7a16
Compare
danceratopz
left a comment
There was a problem hiding this comment.
Thanks @SamWilsn, wonderful, I've missed this check! I have one question in a comment for you below.
I (or someone else) will have to take a closer look at removing the test parametrizations, while it doesn't change functionality, it might reveal a bug in the test.
There was a problem hiding this comment.
More of an update, rather than a review 🙂.
The detected unused parameters in the spec tests are definitely worth investigating and the subsequent clean-up will definitely improve readability.
However, the first case below triggered some investigation as I used hasher and diff'd traces to verify that we weren't changing the test's intent 🙈. I hope the other changes don't modify the test fixtures so that effort remains small.
I'm not a big fan of bundling changes that modify test vectors in what is essentially a "tooling" PR, wondering what your thoughts are @spencer-tb? Should we leave changes to test functions out of this PR and handle them in a follow-up PR? We could mark them here with # noqa: arg001? This would be my preference. Actually I don't mind as long as someone else reviews any changes I make to the tests to get a second pair of eyes :)
If the test fixtures (generated) change "correctly" (doesn't change the functionality of the test) I think a subsequent PR makes sense. |
|
Thanks @spencer-tb! @SamWilsn, could you please revert the changes and apply |
🗒️ Description
Enables the
ARG001lint and fixes the issues it discovers.🔗 Related Issues or PRs
ethereum/execution-specs#1403
✅ Checklist
toxchecks to avoid unnecessary CI fails, see also Code Standards and Enabling Pre-commit Checks:uvx --with=tox-uv tox -e lint,typecheck,spellcheck,markdownlinttype(scope):.