-
Notifications
You must be signed in to change notification settings - Fork 82
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
dev: Add builder pattern for the machine object in tests #558
Conversation
Hi Tim! Thanks for your first PR, let machine = MachineBuilder::new().with_bytecode(bytecode).with_calldata(calldata).with_caller(caller).with_read_only().build(); Or something similar that way, we can reduce overall LoC and have more nimble tests. Moreover, if a test only needs a to set the bytecode, but all the rest can be our pre-defined presets, we can do: let machine = MachineBuilder::new().with_preset().with_bytecode(bytecode).build(); Lmk if you think that's ok @enitrat @edisontim |
agreed - the way it has been implemented just abstracts it but doesn't make it simpler! |
Ok I'll remove the Director object from there then :) |
Makes sense to me to also get rid of |
I think you're right |
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.
yep looks good now!
all good, please rebase! |
4a7c88e
to
c2b82b2
Compare
Done :) ! |
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.
@Eikix LGTM
Pull Request type
Please check the type of change your PR introduces:
What is the current behavior?
Resolves: #546
What is the new behavior?
Add builder pattern for Machine object in tests
Does this introduce a breaking change?