Skip to content

move huge similar _evalue logic into separate methods#341

Merged
danceratopz merged 5 commits intoethereum:mainfrom
winsvega:refactoring
Nov 29, 2023
Merged

move huge similar _evalue logic into separate methods#341
danceratopz merged 5 commits intoethereum:mainfrom
winsvega:refactoring

Conversation

@winsvega
Copy link
Copy Markdown

@winsvega winsvega commented Nov 3, 2023

🗒️ Description

The intent is to split huge files with a lot of logic
Devide the logic into separate files and refactor into a smaller functions
Making the overall code more readable

I do by small steps as I never used python, to get feedback from the team

Copy link
Copy Markdown
Member

@danceratopz danceratopz left a comment

Choose a reason for hiding this comment

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

Thanks, I appreciate the intention! The TransitionTool class and evaluate functions definitely need some clean-up!

Imo, there's not that much to be gained from moving them to their own file (but I think I see where you're going with this). It would def be great to refactor them into smaller functions, but also happy to keep these in transition_tool.py or move them to a utils.py, or something else, as appropriate.

You previously mentioned in a discussion about sub-classing in order to spread a class implementation over separate files. I don't think it's good form to subclass for the evaluate methods just to achieve this goal and this is not typically done in Python.

However, the TransitionTool class is in need of a larger refactor, as we're starting to add interfaces to other EVM tools (such as blocktest, see its verify_fixtures() method) which has nothing to do with t8n tools. So in order to achieve something similar to what I think you'd like to do, we could make an EVMTool base class and then make TransitionTool and FixtureConsumer subclasses (perhaps we can find a better name than FixtureConsumer, but it could potentially interface to both the blocktest and statetest commands).

Few other comments below.

Happy to jump on a call and discuss how to proceed!

Comment thread src/evm_transition_tool/transition_tool_methods/_filesystem.py
Comment thread src/evm_transition_tool/transition_tool_methods/_filesystem.py Outdated
Comment thread src/evm_transition_tool/transition_tool_methods/_filesystem.py Outdated
Comment thread src/evm_transition_tool/transition_tool_methods/_evaluate.py Outdated
@danceratopz danceratopz added scope:evm Scope: evm_transition_tool package type:refactor Type: Refactor labels Nov 3, 2023
Copy link
Copy Markdown
Member

@danceratopz danceratopz left a comment

Choose a reason for hiding this comment

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

LGTM. Could you fix up tox?

And add the missing typehints? That can sometimes be tricky, feel free to reach out!

Comment thread src/evm_transition_tool/transition_tool.py Outdated
Comment thread src/evm_transition_tool/transition_tool.py Outdated
Comment thread src/evm_transition_tool/transition_tool.py Outdated
@winsvega winsvega changed the title move huge similar _evalue logic into separate file move huge similar _evalue logic into separate methods Nov 10, 2023
Copy link
Copy Markdown
Member

@danceratopz danceratopz left a comment

Choose a reason for hiding this comment

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

Thanks!

@danceratopz danceratopz merged commit 313b6c1 into ethereum:main Nov 29, 2023
danceratopz added a commit to danceratopz/execution-spec-tests that referenced this pull request Nov 29, 2023
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

scope:evm Scope: evm_transition_tool package type:refactor Type: Refactor

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants