-
Notifications
You must be signed in to change notification settings - Fork 348
docs: formalize non-Black style rules + ban blanket type: ignore (#237) #1447
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
Open
rainwaters11
wants to merge
3
commits into
ethereum:forks/osaka
Choose a base branch
from
rainwaters11:forks/osaka
base: forks/osaka
Could not load branches
Branch not found: {{ refName }}
Loading
Could not load tags
Nothing to show
Loading
Are you sure you want to change the base?
Some commits from the old base branch may be removed from the timeline,
and old review comments may become outdated.
Open
Changes from all commits
Commits
File filter
Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
There are no files selected for viewing
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
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
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
Original file line number | Diff line number | Diff line change |
---|---|---|
@@ -0,0 +1,111 @@ | ||
# EELS Style Guide | ||
|
||
This document outlines style conventions for the Ethereum Execution Layer Specifications (EELS) that are not automatically enforced by Black formatter. These rules promote code clarity, consistency, and maintainability across the codebase. | ||
|
||
## Function Naming | ||
|
||
Function names should be **short, imperative verbs** in `snake_case` format. Follow [PEP 8](https://peps.python.org/pep-0008/#function-and-variable-names) naming conventions. | ||
|
||
**Good:** | ||
```python | ||
def fetch_state(address: Address) -> Account: | ||
"""Fetch account state from storage.""" | ||
|
||
def validate_block(block: Block) -> None: | ||
"""Validate block structure and contents.""" | ||
|
||
def compute_hash(data: bytes) -> Hash32: | ||
"""Compute hash of input data.""" | ||
``` | ||
|
||
**Avoid:** | ||
```python | ||
def do_fetch(address: Address) -> Account: # Avoid "do_" prefixes | ||
def get_the_state_data(address: Address) -> Account: # Too verbose | ||
def validateBlock(block: Block) -> None: # Use snake_case, not camelCase | ||
``` | ||
|
||
## Error Handling | ||
|
||
Use **exceptions** for protocol-handled errors. Avoid sentinel return values for error conditions. | ||
|
||
**Good:** | ||
```python | ||
def execute_transaction(tx: Transaction) -> Receipt: | ||
if tx.gas_limit < INTRINSIC_GAS: | ||
raise InsufficientGas("Gas limit too low") | ||
# ... execution logic | ||
return receipt | ||
|
||
def withdraw_funds(account: Account, amount: Uint256) -> None: | ||
if account.balance < amount: | ||
raise InsufficientFunds(f"Balance {account.balance} < {amount}") | ||
# ... withdrawal logic | ||
``` | ||
|
||
**Avoid:** | ||
```python | ||
def execute_transaction(tx: Transaction) -> Optional[Receipt]: | ||
if tx.gas_limit < INTRINSIC_GAS: | ||
return None # Don't use None for errors | ||
# ... execution logic | ||
return receipt | ||
``` | ||
|
||
## Runtime Checks | ||
|
||
- Use `ensure(...)` for runtime condition validation | ||
- Use `assert` **only** for: | ||
- Invariants and type assumptions | ||
- Bug detection and debugging | ||
- **Never** for control flow or user-facing validation | ||
|
||
**Good:** | ||
```python | ||
def transfer(sender: Account, recipient: Account, amount: Uint256) -> None: | ||
ensure(sender.balance >= amount, "Insufficient balance") # Runtime validation | ||
ensure(amount > 0, "Transfer amount must be positive") # Runtime validation | ||
|
||
# Internal invariant checks | ||
assert isinstance(sender.balance, Uint256) # Type assumption | ||
assert sender.address != recipient.address # Invariant check | ||
``` | ||
|
||
**Avoid:** | ||
```python | ||
def transfer(sender: Account, recipient: Account, amount: Uint256) -> None: | ||
assert sender.balance >= amount # Don't use assert for user validation | ||
assert amount > 0 # Don't use assert for control flow | ||
``` | ||
|
||
## Type Annotations and Ignores | ||
|
||
- **Never** use blanket `# type: ignore` comments | ||
- If type ignoring is absolutely necessary: | ||
- Include the specific error code: `# type: ignore[error-code]` | ||
- Add a brief one-line comment explaining why | ||
|
||
**Good:** | ||
```python | ||
# Specific error code with explanation | ||
result = unsafe_cast(data) # type: ignore[attr-defined] # Legacy API compatibility | ||
|
||
# Type checker limitation workaround | ||
value = getattr(obj, name, default) # type: ignore[misc] # Dynamic attribute access | ||
``` | ||
|
||
**Avoid:** | ||
```python | ||
result = some_function() # type: ignore # Too broad, no explanation | ||
data = process(input) # type: ignore # No error code specified | ||
``` | ||
|
||
## Enforcement | ||
|
||
These style rules are enforced through: | ||
|
||
- **mypy** with strict configuration for type checking and ignore validation | ||
- **Ruff** linting rules (including PGH003 for blanket type ignore detection) | ||
- Code review processes | ||
|
||
The CI pipeline will reject code that violates these guidelines. |
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
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
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
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
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.
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.
Did you forget to add this file?
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.
Hi Sam,
Just wanted to follow up on the PR. I've pushed the final changes, which should resolve your review comments.
Here's a quick summary of what's included in the latest commits:
The complete STYLE_GUIDE.md has been added to the docs.
The CONTRIBUTING.md file is now updated to reference the new style guide.
The mypy.ini and pyproject.toml files have been enhanced to enforce the new rules.
The .gitignore file has been updated.
Everything should be up to date on the branch now and ready for your review. Thanks!
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.
I still don't see a
STYLE_GUIDE.md
?