-
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
add context to resolver #329
Conversation
will uptake #329 before merge |
Codecov Report
@@ Coverage Diff @@
## main #329 +/- ##
=======================================
Coverage 57.16% 57.16%
=======================================
Files 51 51
Lines 6148 6148
=======================================
Hits 3514 3514
Misses 1974 1974
Partials 660 660
|
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.
While you're here, couple more suggestions I have:
- Rename the
did.Resolution
interface todid.Resolver
. This makes it clear that anyone that implementers will need to implementResolve
. - Rename the
did.Resolver
struct todid.MultiMethodResolver
.
did/key_fuzz_test.go
Outdated
@@ -46,7 +47,7 @@ func FuzzCreateAndResolve(f *testing.F) { | |||
didKey, err := CreateDIDKey(kt, pubKey) | |||
assert.NoError(t, err) | |||
|
|||
doc, err := resolver.Resolve(didKey.String()) | |||
doc, err := resolver.Resolve(context.TODO(), didKey.String()) |
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.
Here and elsewhere. Using TODO signals that the context isn't ready, or unavailable. Did you consider using Background()
?
doc, err := resolver.Resolve(context.TODO(), didKey.String()) | |
doc, err := resolver.Resolve(context.Background(), didKey.String()) |
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.
historically I had used TODO for tests to signify it's not a real context, but happy to switch.
// ResolutionOptions https://www.w3.org/TR/did-spec-registries/#did-resolution-options | ||
type ResolutionOptions any | ||
// ResolutionOption https://www.w3.org/TR/did-spec-registries/#did-resolution-options | ||
type ResolutionOption any |
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.
Whenever I see any
, it's typically a symptom of an API that hasn't been fleshed out yet. In this case, it's unclear the shape of what ResolutionOption
is acceptable by the functions that use it.
Instead, I would define an interface. Mind creating an issue to address?
Separately, This doesn't seem to be used anywhere.
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.
yeah not used anywhere yet...will create an issue
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.
addressed all comments - switched all TODO to Background |
unifies resolution interface between the sdk and service