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

Rewrite deserialization logic to use declarative serde struct style #5

Merged
merged 2 commits into from Oct 12, 2022

Conversation

cpubot
Copy link
Contributor

@cpubot cpubot commented Oct 11, 2022

This PR removes the imperative deserialization logic in favor of the declarative and strongly typed struct style enabled by serde.

You'll note the

#![allow(dead_code)]

Attribute in the deserialization module. We can remove the unused fields once the trie generation is implemented; disabling the warnings for now.

@cpubot cpubot requested a review from BGluth October 11, 2022 21:50
Copy link
Member

@BGluth BGluth left a comment

Choose a reason for hiding this comment

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

Yeah looks good! I don't have much to suggest for changes after going through this a handful of times. The way you're doing the JSON parsing is a huge improvement over the way I was initially attempting to implement it.

We absolutely do not need this since the whole process of cloning and fs operations are more than fast enough, but we might be able to make another major improvement on parsing speed by reading in all of the files asynchronously. Might be kind of cool but definitely not needed. 👍

eth_test_parser/Cargo.toml Outdated Show resolved Hide resolved
eth_test_parser/src/fs_scaffolding.rs Show resolved Hide resolved
@cpubot cpubot merged commit 5f3b741 into main Oct 12, 2022
@cpubot cpubot deleted the zbrown/rewrite-deserializer branch October 12, 2022 19:05
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.

None yet

2 participants