-
Notifications
You must be signed in to change notification settings - Fork 15
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
Refactor ProofChain to use builder pattern #39
Comments
The builder pattern is most suitable in cases where the following conditions are met:
In this case, the builder pattern reduces verbosity for the user, sparing them from the mechanical task of filling out a bunch of implicit field values. Can you go into more detail about how the builder pattern enables better recursion? |
@cdata for me, in looking at it, it was also the fact that a proofchain was dependent on the creation of a parser and store, and this PR makes that way more explicit, at least that's what I was going for. I initially did think about defaulting to the memory store for the backend, if that was considered a good move. |
@cdata as far as the recursion goes, I think the build call is cleaner in this variant, as it pushes validation to the use of the parser, but you can look at the PR to see. |
@cdata the other case for the builder is when construction has side effects, and this makes explicit what the pieces are. |
What utility does a user of this interface get from the intermediate stages of construction being exposed here? |
@cdata mainly it was making the creation of a proof chain from a ucan string or CID more explicit in its reliance on the parser and store. I think there may be a better abstraction overall, so totally ok to let this go (minus some minor cleaned-up API bits in this PR), but in reading the code, proof chain creation from string/CID seemed to make the store and parser as implicit side effects. |
I think you may be on to something, but I don't fully understand what you mean by this; are you saying that because you need these things in the invocation, you're implicitly requiring their instantiation at the time we construct a FWIW in the practical usage within Noosphere, both the |
@cdata well, you can create the proofchain in parts, but even with the store and parser in advance, this concept still works. It's more that a proofchain builder makes the need for them to exist somewhere in the code extremely explicit. |
Note: This would be an API change
A Proofchain is always instantiated via ucan/cid and store, parser (to be soon replaced by the resolver). Upon further inspection, it seems better to use a builder pattern for this, where bare instantiation can follow from typical from/try_from methods, like so:
While somewhat more verbose in use, this allows for simpler recursion patterns, and seems to better associate store+parser w/ the proofchain created, for which they rely on.
Thoughts? PoC here: https://github.com/ucan-wg/rs-ucan/pull/38/files.
The text was updated successfully, but these errors were encountered: