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

proofchain w/ builder pattern + minor convert updates #38

Open
wants to merge 1 commit into
base: main
Choose a base branch
from

Conversation

zeeshanlakhani
Copy link
Contributor

@zeeshanlakhani zeeshanlakhani commented Oct 25, 2022

Personally, I felt that proofchain building felt more like a series of steps rather than one fn, b/c it always applied the parser (eventually TODO resolver) and store. This does keep the parser and store attached to a chain, but as private fields, not publically accessible. It's a bit more verbose, but also validates the parser in a separate step, which I like.

In Draft mode, associated w/ issue #39.

Includes:

@zeeshanlakhani zeeshanlakhani requested review from cdata and a team as code owners October 25, 2022 18:28
@zeeshanlakhani zeeshanlakhani force-pushed the zl/refactor-proofchain-to-builder branch from 6875d71 to e4d1bfd Compare October 25, 2022 18:31
@zeeshanlakhani zeeshanlakhani marked this pull request as draft October 25, 2022 18:40
@zeeshanlakhani zeeshanlakhani force-pushed the zl/refactor-proofchain-to-builder branch 3 times, most recently from 9f51bba to df35d9e Compare October 26, 2022 18:38
@zeeshanlakhani zeeshanlakhani force-pushed the zl/refactor-proofchain-to-builder branch from df35d9e to 3c19e8a Compare October 28, 2022 10:16
@zeeshanlakhani zeeshanlakhani marked this pull request as ready for review November 4, 2022 01:30
@zeeshanlakhani
Copy link
Contributor Author

@cdata just wanted to check-in on this one.

@zeeshanlakhani zeeshanlakhani force-pushed the zl/refactor-proofchain-to-builder branch from 3c19e8a to 1104a41 Compare December 1, 2022 17:16
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