-
Notifications
You must be signed in to change notification settings - Fork 329
docs: refactor docstrings to md #1297
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Conversation
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Mostly just these comments, repeated. Looking good so far though!
Hash ([`keccak256`]) of the parent block's header, encoded with [RLP]. | ||
|
||
[`keccak256`]: ref:ethereum.crypto.hash.keccak256 | ||
[RLP]: https://ethereum.github.io/ethereum-rlp/src/ethereum_rlp/rlp.py.html | ||
Hash | ||
([`keccak256`](../crypto/hash.py.html#ethereum.crypto.hash.keccak256:0)) | ||
of the parent block's header, encoded with | ||
[RLP](https://ethereum.github.io/ethereum-rlp/src/ethereum_rlp/rlp.py.html). |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Looks like these docstrings got converted from markdown back into markdown, while losing the docc features (eg. ref:ethereum.crypto.hash.keccak256
)? I'm pretty sure you already did blocks.py
and transactions.py
in #1263.
from included transactions. Base fees (introduced in | ||
[EIP-1559](https://eips.ethereum.org/EIPS/eip-1559)) are burned and do not | ||
go to the coinbase. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
We should try to avoid making changes without material effect because it makes tracing history (like with git blame
) more difficult. Here we're just changing from footnote style links to inline, but the rendered output would be the same.
Ethereum Logs Bloom | ||
^^^^^^^^^^^^^^^^^^^ | ||
# Ethereum Logs Bloom | ||
|
||
.. contents:: Table of Contents | ||
:backlinks: none | ||
:local: | ||
|
||
Introduction | ||
------------ | ||
## Introduction | ||
|
||
This modules defines functions for calculating bloom filters of logs. For the | ||
general theory of bloom filters see e.g. `Wikipedia | ||
<https://en.wikipedia.org/wiki/Bloom_filter>`_. Bloom filters are used to allow | ||
for efficient searching of logs by address and/or topic, by rapidly | ||
eliminating blocks and receipts from their search. | ||
general theory of bloom filters see e.g. | ||
[Wikipedia](https://en.wikipedia.org/wiki/Bloom_filter). Bloom filters are | ||
used to allow for efficient searching of logs by address and/or topic, by | ||
rapidly eliminating blocks and receipts from their search. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This is definitely more what I had in mind for this issue!
@@ -1,19 +1,13 @@ | |||
""" | |||
Ethereum Logs Bloom | |||
^^^^^^^^^^^^^^^^^^^ | |||
# Ethereum Logs Bloom |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Because docc automatically adds a header with the module name, we can omit these manual ones.
# Ethereum Logs Bloom |
|
||
Introduction | ||
------------ | ||
## Introduction |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Until we have need for more subsections, how about we drop this one too and go directly into the content?
## Introduction |
@@ -35,12 +29,9 @@ def add_to_bloom(bloom: bytearray, bloom_entry: Bytes) -> None: | |||
least significant 11 bits from the first 3 16-bit words of the | |||
`keccak_256()` hash of `bloom_entry`. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
While we're here, we could fix the name and maybe add a link?
`keccak_256()` hash of `bloom_entry`. | |
[`keccak256`] hash of `bloom_entry`. | |
[`keccak256`]: ref:ethereum.crypto.hash.keccak256 |
#### Parameters | ||
- bloom: The bloom filter. | ||
- bloom_entry: An entry which is to be added to bloom filter. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
#### Parameters | |
- bloom: The bloom filter. | |
- bloom_entry: An entry which is to be added to bloom filter. |
I'm okay just dropping the parameters sections and adding some prose if the parameters are confusing. Most of the time the meaning of the parameter is obvious from the name anyway.
Unknown [EIP-2718](https://eips.ethereum.org/EIPS/eip-2718) transaction | ||
type byte. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Another unintentional markdown -> markdown?
#### Returns | ||
- logs_bloom: `Bloom` | ||
The logs bloom obtained which is 256 bytes with some bits set as per the | ||
caller address and the log topics. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Similar to the parameters section, I think we can drop returns and incorporate the text. Something like:
Obtain the logs bloom from a list of log entries.
The address and each topic of a log are collected into a bloom filter and returned.
@@ -95,15 +89,12 @@ def apply_fork(old: BlockChain) -> BlockChain: | |||
is used to handle the irregularity. See the :ref:`DAO Fork <dao-fork>` for |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This reStructuredText didn't get converted. I think it would look something like:
is used to handle the irregularity. See the :ref:`DAO Fork <dao-fork>` for | |
is used to handle the irregularity. See the [DAO Fork](ref:ethereum.dao_fork) for |
What was wrong?
How was it fixed?
docstrings were updated from restructured text to markdown
Cute Animal Picture