-
Notifications
You must be signed in to change notification settings - Fork 55
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 IsValid() in did/web/web.go #399
Refactor IsValid() in did/web/web.go #399
Conversation
nice to see a test case too, cool |
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.
Thanks so much! A couple minor nits
did/web/web.go
Outdated
func (d DIDWeb) Validate(ctx context.Context) error { | ||
_, err := d.resolveDocBytes(ctx) | ||
if err != nil { | ||
return errors.Wrap(err, "failed to validate DID") |
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.
return errors.Wrap(err, "failed to validate DID") | |
return errors.Wrap(err, "resolving doc bytes") |
did/web/web_test.go
Outdated
t.Run("Unresolvable Path - Validate DID", func(tt *testing.T) { | ||
err := didWebNotADomain.Validate(context.Background()) | ||
assert.Error(tt, err) | ||
assert.Contains(tt, err.Error(), "failed to validate") |
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.
Prefer using
assert.Contains(tt, err.Error(), "failed to validate") | |
assert.ErrorContains(tt, err, "failed to validate") |
Codecov Report
@@ Coverage Diff @@
## main #399 +/- ##
==========================================
+ Coverage 57.22% 57.26% +0.04%
==========================================
Files 68 68
Lines 7347 7354 +7
==========================================
+ Hits 4204 4211 +7
+ Misses 2400 2399 -1
- Partials 743 744 +1
|
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.
very nice. thank you!
Looks like this PR now has conflicts with the mainline. I have just made an attempt to resolve them from the Github UI. |
Description
IsValid()
toerror
, and rename the function toValidate()
.context.Context
as it's first parameter and propagate it to the network call.Validate()
function.Testing
Ran
mage cbt
without any errors.References
Related to #395 .