-
Notifications
You must be signed in to change notification settings - Fork 54
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
Adding DID Document Builder #284
Conversation
Codecov Report
@@ Coverage Diff @@
## main #284 +/- ##
==========================================
+ Coverage 60.43% 60.54% +0.11%
==========================================
Files 40 41 +1
Lines 4458 4554 +96
==========================================
+ Hits 2694 2757 +63
- Misses 1330 1352 +22
- Partials 434 445 +11
|
@decentralgabe can you take a look and merge if this is ok? |
return errors.Wrap(err, "malformed context") | ||
} | ||
uniqueContexts := util.MergeUniqueValues(builder.contexts, res) | ||
builder.contexts = uniqueContexts |
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.
Why doe we have an outer layer with contexts as well?
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.
great question. I used https://github.com/TBD54566975/ssi-sdk/blob/main/credential/builder.go as a reference. The reason it was suggested was the following:
On : https://github.com/TBD54566975/ssi-sdk/blob/main/credential/builder.go#L25
// contexts and types are kept to avoid having cast to/from interface{} values
I made an assumption here since the context is also the same data type, this is a reasonable implementation to rely upon for did document builder. The reason I would give is similar to the above. I can put in comments if helpful
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.
supportive of this!
Co-authored-by: Gabe <7622243+decentralgabe@users.noreply.github.com>
Thanks for the review @decentralgabe . addressed the comments. Realized you might be busy. No rush here. @nitro-neal lmk if your concerns are addressed as well. Thanks guys! |
bonk. @decentralgabe or @nitro-neal can you get back and possible merge if you're good with this? |
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.
I understand that this is following the same pattern that existed, but it is strange to me that every setter method returns an error, which can only be the BuilderEmptyError. I think it would be cleaner for only Build
to return an error. The rest should simply accumulate changes.
// Builds the DID Document | ||
func (builder *DIDDocumentBuilder) Build() (*DIDDocument, error) { | ||
if builder.IsEmpty() { | ||
return nil, errors.New(BuilderEmptyError) |
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.
Good practice for the error to be the constant, so callers can compare against it. You can also reuse.
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.
@andresuribe87 understand. Following existing implementation here https://github.com/TBD54566975/ssi-sdk/blob/main/credential/builder.go#L70 to stay consistent. If we want to revisit how errors are handle in the builders I'm OK with that, but I think they should be consistent across the VC builder and the DIDDocument builder.
return reflect.DeepEqual(builder, &DIDDocumentBuilder{}) | ||
} | ||
|
||
func (builder *DIDDocumentBuilder) AddContext(context interface{}) error { |
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.
func (builder *DIDDocumentBuilder) AddContext(context interface{}) error { | |
func (builder *DIDDocumentBuilder) AddContext(context any) error { |
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.
@andresuribe87 as long as this works, I'm fine moving to any, but note https://github.com/TBD54566975/ssi-sdk/blob/main/credential/builder.go#L68 uses the interface{}
type.
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.
Either works, filed #289
) | ||
|
||
// Exercise all builder methods | ||
func TestDIDDocumentBuilder(t *testing.T) { |
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.
Would it make sense to split this test into multiple smaller ones, that exercise a single behavior?
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.
Reasonable suggestion if we want to update that. I can put a note in for a future PR to move this into smaller tests.
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.
Why not do it in this PR?
From a maintainability perspective, it's easier to have smaller, self contained tests.
FWIW, I want to add coding guidelines into the contribution doc. See #290
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.
@andresuribe87 again, this was all using the reference of the VC Builder code, which does tests in a similar way. https://github.com/TBD54566975/ssi-sdk/blob/main/credential/builder_test.go#L254. I was being consistent with the existing codebase. I think if we want to break things out, we should also address the VC Builder as well.
There are good reasons to have smaller tests, but IMO this shouldn't block the PR.
type DIDDocumentBuilder struct { | ||
contexts []string | ||
types []string | ||
*DIDDocument |
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.
Why have this be exposed to users of the DIDDocumentBuilder
class?
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.
@andresuribe87 I was following precedence from previous code implemented in the ssi-sdk: https://github.com/TBD54566975/ssi-sdk/blob/main/credential/builder.go#L24.
Co-authored-by: Andres Uribe <andresuribe87@gmail.com>
thanks @andorsk ! |
Thanks @andresuribe87 @nitro-neal @decentralgabe for the review! |
In reference to #283