chore: expand docstring guide#2195
Merged
Merged
Conversation
danceratopz
reviewed
Feb 16, 2026
danceratopz
left a comment
Member
There was a problem hiding this comment.
Hey @SamWilsn, thanks for this! I made a few comments below, which I tried to address in:
This PR also revamps the top-level "Docstring" section bullet list. Please see the commit messages for more details.
Comment on lines
88
to
92
| @@ -91,6 +91,20 @@ This specification aims to be: | |||
| - Write in complete sentences, providing background and context for the associated code. | |||
| - Link to relevant standards/EIPs. | |||
Member
There was a problem hiding this comment.
I think it'd be good to re-order this and put the "don't"s together
Comment on lines
88
to
92
| @@ -91,6 +91,20 @@ This specification aims to be: | |||
| - Write in complete sentences, providing background and context for the associated code. | |||
| - Link to relevant standards/EIPs. | |||
Member
There was a problem hiding this comment.
We prefer reference-style links, I think it's worth mentioning.
We should update this when we have:
|
|
||
| ##### Constants | ||
|
|
||
| - Do not include the value of the constant in the docstring (not verbatim and not a decomposition). It's too easy to change the constant and forget to change the docstring. |
Member
There was a problem hiding this comment.
"not verbatim and not a decomposition" feels a bit clunky.
| - Do not include the value of the constant in the docstring (not verbatim and not a decomposition). It's too easy to change the constant and forget to change the docstring. | ||
| - Construct the constant's value from more basic components when doing so provides meaningful context. | ||
| - **Great:** `TARGET_BLOB_GAS_PER_BLOCK = GAS_PER_BLOB * BLOB_SCHEDULE_TARGET` | ||
| - **Good:** `TX_MAX_GAS = Uint(2 ** 24)` |
Member
There was a problem hiding this comment.
I would argue that this isn't really a "good" example, but rather "acceptable" as it still refers to a literal and goes against the above advice. But I think it's ok in this case!
| - Construct the constant's value from more basic components when doing so provides meaningful context. | ||
| - **Great:** `TARGET_BLOB_GAS_PER_BLOCK = GAS_PER_BLOB * BLOB_SCHEDULE_TARGET` | ||
| - **Good:** `TX_MAX_GAS = Uint(2 ** 24)` | ||
| - **Bad:** `TX_MAS_GAS = Uint(16_777_216)` |
Member
There was a problem hiding this comment.
I guess the "mas" typo in TX_MAS_GAS was unintentional even though it's a bad example.
| ##### Functions | ||
|
|
||
| - The first paragraph should be a brief summary of what the function does, written in the imperative mood. | ||
| - **Good:** Build the house using the provided lumber. |
* chore(docs): reword constants docstring guidance Clarify the prohibition on including constant values in docstrings by replacing the parenthetical "(not verbatim and not a decomposition)" with more precise language. * chore(docs): relabel constant examples and add justifications Relabel "Good" to "Acceptable" for the literal expression example, reword the composition guidance, add explanations for each example tier, and fix TX_MAS_GAS typo. * chore(docs): revamp top-level docstring guidelines - Reorder bullets to lead with positive guidance and group "don'ts" at the end. - Add one-line vs multi-line preference with examples. - Add reference-style link example for EIP references. * chore(docs): promote imperative mood guideline to top-level - Move the imperative mood bullet from the Functions subsection into the top-level docstring list, next to the one-liner preference. - Remove the now-empty Functions subsection. # Conflicts: # CONTRIBUTING.md * chore(docs): clean-up some whitespace * chore(docs): address review suggestions * Update CONTRIBUTING.md * Update CONTRIBUTING.md * Update CONTRIBUTING.md --------- Co-authored-by: Sam Wilson <57262657+SamWilsn@users.noreply.github.com>
kevaundray
added a commit
to kevaundray/execution-specs
that referenced
this pull request
Feb 28, 2026
commit a48c892 Author: Kevaundray Wedderburn <kevtheappdev@gmail.com> Date: Fri Feb 27 17:39:02 2026 +0000 introduce BlockDiff commit a4a4b47 Merge: e801c45 5034aa2 Author: Kevaundray Wedderburn <kevtheappdev@gmail.com> Date: Tue Feb 24 23:25:33 2026 +0000 Merge remote-tracking branch 'upstream/forks/amsterdam' into kw/extract-stateless-functionality commit e801c45 Author: Kevaundray Wedderburn <kevtheappdev@gmail.com> Date: Tue Feb 24 23:14:11 2026 +0000 add doc comments for ChainContext fields commit de68f63 Author: Kevaundray Wedderburn <kevtheappdev@gmail.com> Date: Tue Feb 24 22:57:06 2026 +0000 initial commit commit 5034aa2 Author: kevaundray <kevtheappdev@gmail.com> Date: Tue Feb 24 22:56:44 2026 +0000 chore(ci): Skip diffs if not on default branch (ethereum#2304) * skip diffs * Update .github/workflows/gh-pages.yaml Co-authored-by: Sam Wilson <57262657+SamWilsn@users.noreply.github.com> * revert: actionlint --------- Co-authored-by: Sam Wilson <57262657+SamWilsn@users.noreply.github.com> commit 7ca499e Author: danceratopz <danceratopz@gmail.com> Date: Tue Feb 24 20:06:22 2026 +0100 feat(test-fill,test-benchmark): organize fixtures into fork and gas-limit subdirs (ethereum#2134) * feat(test-benchmark): split gas benchmark outputs by subdir * docs(test-benchmark): document gas benchmark output layout * test(test-benchmark): verify gas benchmark subdir layout * feat(test-benchmark): update folder structure according to review * refactor(test-filler): move fixture_output to shared Move FixtureOutput from filler/ to shared/ and absorb fixture_naming.py. This prepares for future build-tool plugins that also generate fixtures. * feat(test-filler): organize fill fixture output by target fork Always include the gas-limit suffix in the fork output subdirectory, not just for benchmark tests. Non-benchmark (consensus) tests now also get a subdirectory like `for_prague_at_0120M/` using the environment default gas limit (respecting `--block-gas-limit`). The `_at_` separator makes the directory name unambiguous: for_{fork}_at_{gas}M (e.g. for_prague_at_0120M) * docs(releases): document fixture output directory layout Add a "Fixture Output Directory Structure" section explaining the for_{fork}_at_{gas}M subdirectory convention, default gas limit, and how benchmark tests use --gas-benchmark-values. * feat(test-filler): drop gas limit from consensus fixture subdirs Consensus fixtures now use `for_{fork}/` (e.g. `for_prague/`) instead of `for_{fork}_at_{gas}M/`. Benchmark fixtures keep the gas limit suffix. * feat(test-filler): use fixture_subfolder marker for output subdirs Replace manual gas-benchmark detection in the filler with a composable pytest.mark.fixture_subfolder(level, prefix) marker: - Add level-0 marker (fork prefix) in fork parametrization. - Add level-1 marker in GasBenchmarkValues and OpcodeCountsConfig. - Resolve output subdirs from markers in filler instead of hard-coding. - Break circular import so collector can share the separator constant. - Register the marker in execute_fill alongside other shared markers. * feat(test-tests): add fixed-opcode-count subdirectory output test - Verify --fixed-opcode-count splits fixtures into per-count subdirs. - Mirror the existing gas-benchmark subdir test for opcode counts. * fix(test-fill): fix-up typehints for mypy * fix(test-fill): remove dead code and tighten types - Remove unused `should_auto_enable_all_formats` property. - Remove unused `format_gas_limit_subdir()` function. - Remove unreachable `FORK_SUBDIR_PREFIX` check in post-merge verification (format dir is always at `parts[0]`). - Tighten `resolve_fixture_subfolder` parameter from `list` to `list[pytest.Mark]`. - Clarify "benchmark" prefix stripping in collector with comment. * docs(test-fill): update docs for fixed-opcode-count fixture dir layout * fix: static tests --------- Co-authored-by: Mario Vega <marioevz@gmail.com> commit 84025b7 Author: Sam Wilson <57262657+SamWilsn@users.noreply.github.com> Date: Tue Feb 24 14:05:19 2026 -0500 chore: bump docc version (ethereum#2303) commit 4fff5eb Author: Mario Vega <marioevz@gmail.com> Date: Tue Feb 24 19:30:35 2026 +0100 feat(test-execute): Support `testing_buildBlockV1` (ethereum#2295) * feat(test-rpc): Add `Testing` namespace * nit: transactions parameter * feat(test-execute): Support `testing_buildBlockV1` * docs * refactor(execute): Testing endpoint non-auth * review comments Co-authored-by: fselmo <fselmo2@gmail.com> --------- Co-authored-by: fselmo <fselmo2@gmail.com> commit 7329095 Author: Carson <carson@ethereum.org> Date: Tue Feb 24 12:29:35 2026 -0500 refactor(test): rename/align gas cost constants (ethereum#2094) - rename base gas cost - rename very low gas cost - rename memory gas cost - rename copy gas cost - rename low gas cost - rename medium gas cost - rename high gas cost - rename warm access gas cost - rename cold access gas cost - rename storage reset gas cost - rename storage clear gas cost - rename storage set gas cost - rename warm account gas cost - rename cold account gas cost - rename initcode word gas cost - rename code deposit gas cost - rename create gas cost - rename call stipen gas cost - rename selfdestruct gas cost - rename call value gas cost - rename new account gas cost - rename exp gas cost - rename exp per byte gas cost - rename keccak256 gas cost - rename keccak256 word gas cost - rename blockhash gas cost - rename log data gas cost - rename log gas cost - rename log topic gas cost - rename tx based gas cost - rename contract creation gas cost - rename access list addr gas cost - rename access list storage gas cost - rename floor calldata gas cost - rename token standard gas cost - rename authorize gas cost - rename authorize base cost - rename data zero gas cost - rename non-zero data gas cost - rename blob gas per block gas cost - rename blob min price gas cost - rename all precompile gas costs - (refactor) TX_ACCESS_LIST_ADDRESS_COST => GAS_TX_ACCESS_LIST_ADDRESS - (refactor) GAS_CODE_DEPOSIT => GAS_CODE_DEPOSIT_PER_BYTE - (refactor) remove unnecessary Uint cast for REFUND_STORAGE_CLEAR - (refactor) TX_ACCESS_LIST_STORAGE_KEY_COST => GAS_TX_ACCESS_LIST_STORAGE_KEY - (refactor) GAS_KECCAK256_WORD => GAS_KECCAK256_PER_WORD - (refactor) TX_BASE_COST => GAS_TX_BASE - (refactor) TX_CREATE_COST => GAS_TX_CREATE - (refactor) TX_DATA_STANDARD_TOKEN_COST => GAS_TX_DATA_TOKEN_STANDARD - (refactor) => TX_DATA_FLOOR_TOKEN_COST => GAS_TX_DATA_TOKEN_FLOOR - (refactor) GAS_INIT_CODE_WORD_COST => GAS_CODE_INIT_PER_WORD - (refactor) TX_DATA_COST_PER_ZERO => GAS_TX_DATA_PER_ZERO - (refactor) TX_DATA_COST_PER_NON_ZERO => GAS_TX_DATA_PER_NON_ZERO - (refactor) GAS_AUTH_PER_EMPTY_ACCOUNT_COST => GAS_AUTH_PER_EMPTY_ACCOUNT - (refactor) GAS_LOG_DATA => GAS_LOG_DATA_PER_BYTE - (refactor) GAS_PRECOMPILE_SHA256_WORD => GAS_PRECOMPILE_SHA256_PER_WORD - (refactor) GAS_PRECOMPILE_RIPEMD160_WORD => GAS_PRECOMPILE_RIPEMD160_PER_WORD - (refactor) GAS_PRECOMPILE_IDENTITY_WORD => GAS_PRECOMPILE_IDENTITY_PER_WORD - (refactor) REFUND_PER_AUTH_BASE_COST => REFUND_AUTH_PER_EXISTING_ACCOUNT --------- Co-authored-by: LouisTsai <q1030176@gmail.com> commit cdf9add Author: Paweł Bylica <pawel@hepcolgum.band> Date: Tue Feb 24 16:53:55 2026 +0100 feat(tests): port EXTCODEHASH dynamicAccountOverwriteEmpty (ethereum#2032) Port the legacy static test: stExtCodeHash/dynamicAccountOverwriteEmpty_Paris for EIP-1052 EXTCODEHASH testing. The test verifies EXTCODEHASH correctly updates when an empty account is overwritten by CREATE2. Modified from original: target account has no storage to avoid EIP-7610 collision behavior. commit 9bd111a Author: felix <felix314159@users.noreply.github.com> Date: Tue Feb 24 14:20:18 2026 +0100 fix: failing test in ci for 7981 (ethereum#2297) commit 6f80f72 Author: Jochem Brouwer <jochembrouwer96@gmail.com> Date: Tue Feb 24 10:25:24 2026 +0100 refactor(test-benchmarks): ensure `SSTORE` N->N writes nonzero key/value (ethereum#2255) commit 04fb242 Author: Paweł Bylica <pawel@hepcolgum.band> Date: Mon Feb 23 23:24:31 2026 +0100 feat(tests): port EXTCODEHASH codeless account with storage test (ethereum#2291) Port sub-case from dynamicAccountOverwriteEmpty_ParisFiller.yml: a codeless account with storage returns keccak256("") for EXTCODEHASH and 0 for EXTCODESIZE. Parametrized over three non-empty variants (balance only, nonce only, both). commit 7add493 Author: Sam Wilson <57262657+SamWilsn@users.noreply.github.com> Date: Mon Feb 23 15:45:25 2026 -0500 chore: expand docstring guide (ethereum#2195) Co-authored-by: danceratopz <danceratopz@gmail.com>
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
2 participants
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.
Cute Animal Picture