Skip to content
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

bump mypy to run locally, move tools/factories into tests folder #2197

Open
wants to merge 2 commits into
base: main
Choose a base branch
from

Conversation

pacrob
Copy link
Collaborator

@pacrob pacrob commented Feb 4, 2025

What was wrong?

mypy was still running without locally installed deps.
factory-boy is only used in testing, but tools using it were within a public folder.

How was it fixed?

Run mypy locally - decided to do this here, as opposed to with the template update, to keep related change more clearly separated.

Moved eth/tools/factories to tests/tools/factories.

Todo:

  • Clean up commit history
  • Add or update documentation related to these changes
  • Add entry to the release notes

Cute Animal Picture

image

@pacrob pacrob self-assigned this Feb 4, 2025
@pacrob pacrob force-pushed the move-factories-to-tests branch 6 times, most recently from ddf2cf2 to d371a13 Compare February 12, 2025 21:40
TransactionBuilderAPI,
WithdrawalAPI,
)


class BaseBlock(Configurable, rlp.Serializable, BlockAPI):
transaction_builder: Type[TransactionBuilderAPI] = None

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

This was added to clarify that it is the rlp.Serializable's __init__ that we want, not BlockAPI's. At runtime, the correct one was being chosen by default, but mypy was confused. And I think it's better to be explicit about it.

@pacrob pacrob force-pushed the move-factories-to-tests branch from d371a13 to 77a8d0c Compare February 12, 2025 21:49
@pacrob pacrob requested review from fselmo, reedsa and kclowes February 12, 2025 22:06
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

1 participant