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
feat(dispatch): Cursor for easy pagination #1678
Conversation
Registering MethodSets compares the MethodSet against the implementation to make sure they match each other. Add tests for dispatch to ensure common failure cases actually work.
Func implementations registered with Dispatch can return 1-3 values. Only 1 value means it must be an error. 2 values means the first is any output and the second is an error. 3 values means the first is any output, the second is a Cursor, and the third is an error. Cursor will be used for pagination of methods that support it.
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.
some comment nits, but this is fantastic!
@@ -95,8 +95,7 @@ func (o *CheckoutOptions) Run() (err error) { | |||
return err | |||
} | |||
|
|||
_, err = inst.Filesys().Checkout(ctx, &lib.LinkParams{Dir: o.Dir, Refstr: ref}) | |||
if err != nil { | |||
if err = inst.Filesys().Checkout(ctx, &lib.LinkParams{Dir: o.Dir, Refstr: ref}); err != nil { |
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.
lovely
lib/dispatch.go
Outdated
// as the input and output parameters for those methods, and associates a string name for each | ||
// method. Dispatch works by looking up that method name, constructing the necessary input, | ||
// then invoking the actual implementation. |
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.
mind adjusting the comment to talk about how dispatch returns a non-nil cursor for paginated responses
?
lib/dispatch_test.go
Outdated
// Test data: methodSet and implementation | ||
|
||
type animalMethods struct { |
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.
it's not "effective go" to label sections of code with comments. add these comments to the definition of each struct by (for example) removing line 100
"testing" | ||
) | ||
|
||
func TestRegisterMethods(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.
thank you for writing these!
// There are either 1, 2, or 3 output values: | ||
// 1: func() (err) | ||
// 2: func() (out, err) | ||
// 3: func() (out, cur, err) |
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.
We should get this comment in a place where MethodSet
implementers can find it. I think a big 'ol comment above the methodSet interface
Also, tests gotta pass 😄 |
All comments addressed, merging now! |
No description provided.