Skip to content

Commit 8f6509b

Browse files
committed
fix(lib): Improve context passing and visibility of internal structs
1 parent afaf06d commit 8f6509b

File tree

10 files changed

+62
-75
lines changed

10 files changed

+62
-75
lines changed

cmd/checkout.go

Lines changed: 1 addition & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -71,7 +71,7 @@ func (o *CheckoutOptions) Complete(f Factory, args []string) (err error) {
7171

7272
// Run executes the `checkout` command
7373
func (o *CheckoutOptions) Run() (err error) {
74-
ctx := context.Background()
74+
ctx := context.TODO()
7575
inst := o.Instance
7676

7777
if !o.Refs.IsExplicit() {

cmd/fsi.go

Lines changed: 4 additions & 4 deletions
Original file line numberDiff line numberDiff line change
@@ -45,7 +45,8 @@ func NewFSICommand(f Factory, ioStreams ioes.IOStreams) *cobra.Command {
4545
if err := o.Complete(f, args); err != nil {
4646
return err
4747
}
48-
return o.Unlink()
48+
ctx := context.TODO()
49+
return o.Unlink(ctx)
4950
},
5051
}
5152

@@ -87,7 +88,7 @@ func (o *FSIOptions) Link() (err error) {
8788
return err
8889
}
8990

90-
ctx := context.Background()
91+
ctx := context.TODO()
9192
inst := o.Instance
9293

9394
p := &lib.LinkParams{
@@ -104,8 +105,7 @@ func (o *FSIOptions) Link() (err error) {
104105
}
105106

106107
// Unlink executes the fsi unlink command
107-
func (o *FSIOptions) Unlink() error {
108-
ctx := context.Background()
108+
func (o *FSIOptions) Unlink(ctx context.Context) error {
109109
inst := o.Instance
110110

111111
for _, ref := range o.Refs.RefList() {

cmd/init.go

Lines changed: 1 addition & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -88,7 +88,7 @@ func (o *InitOptions) Complete(f Factory, args []string) (err error) {
8888

8989
// Run executes the `init` command
9090
func (o *InitOptions) Run() (err error) {
91-
ctx := context.Background()
91+
ctx := context.TODO()
9292
inst := o.Instance
9393

9494
// An empty dir means the current dir

cmd/restore.go

Lines changed: 1 addition & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -124,7 +124,7 @@ func (o *RestoreOptions) Complete(f Factory, args []string) (err error) {
124124
func (o *RestoreOptions) Run() (err error) {
125125
printRefSelect(o.ErrOut, o.Refs)
126126

127-
ctx := context.Background()
127+
ctx := context.TODO()
128128
inst := o.Instance
129129

130130
ref := o.Refs.Ref()

cmd/status.go

Lines changed: 1 addition & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -86,7 +86,7 @@ const ColumnPositionForMtime = 40
8686
func (o *StatusOptions) Run() (err error) {
8787
printRefSelect(o.ErrOut, o.Refs)
8888

89-
ctx := context.Background()
89+
ctx := context.TODO()
9090
inst := o.Instance
9191

9292
params := lib.LinkParams{Dir: o.Refs.Dir()}

cmd/whatchanged.go

Lines changed: 1 addition & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -52,7 +52,7 @@ func (o *WhatChangedOptions) Complete(f Factory, args []string) (err error) {
5252
func (o *WhatChangedOptions) Run() (err error) {
5353
printRefSelect(o.ErrOut, o.Refs)
5454

55-
ctx := context.Background()
55+
ctx := context.TODO()
5656
inst := o.Instance
5757

5858
params := lib.LinkParams{Refstr: o.Refs.Ref()}

lib/dispatch.go

Lines changed: 25 additions & 30 deletions
Original file line numberDiff line numberDiff line change
@@ -7,7 +7,7 @@ import (
77
"strings"
88
)
99

10-
// Dispatch is system for handling calls to lib. Currently only implemented for FSI methods.
10+
// Dispatch is a system for handling calls to lib. Should only be called by top-level lib methods.
1111
//
1212
// When programs are using qri as a library (such as the `cmd` package), calls to `lib` will
1313
// arrive at dispatch, before being routed to the actual implementation routine. This solves
@@ -22,13 +22,6 @@ import (
2222
// as the input and output parameters for those methods, and associates a string name for each
2323
// method. Dispatch works by looking up that method name, constructing the necessary input,
2424
// then invoking the actual implementation.
25-
26-
// RegMethodSet represents a set of registered methods
27-
type RegMethodSet struct {
28-
reg map[string]callable
29-
}
30-
31-
// Dispatch looks up the method, and invokes it with the given input
3225
func (inst *Instance) Dispatch(ctx context.Context, method string, param interface{}) (res interface{}, err error) {
3326
if inst == nil {
3427
return nil, fmt.Errorf("instance is nil, cannot dispatch")
@@ -60,12 +53,12 @@ func (inst *Instance) Dispatch(ctx context.Context, method string, param interfa
6053
// TODO(dustmop): Add user authentication, profile, identity, etc
6154
// TODO(dustmop): Also determine if the method is read-only vs read-write,
6255
// and only execute a single read-write method at a time
63-
// Eventually, the data that lives in Scope should be immutable for its lifetime,
56+
// Eventually, the data that lives in scope should be immutable for its lifetime,
6457
// or use copy-on-write semantics, so that one method running at the same time as
6558
// another cannot modify the out-of-scope data of the other. This will mostly
6659
// involve making copies of the right things
67-
scope := Scope{
68-
ctx: ctx,
60+
scope := scope{
61+
ctx: ctx,
6962
inst: inst,
7063
}
7164

@@ -110,8 +103,13 @@ func (inst *Instance) NewInputParam(method string) interface{} {
110103
return nil
111104
}
112105

106+
// regMethodSet represents a set of registered methods
107+
type regMethodSet struct {
108+
reg map[string]callable
109+
}
110+
113111
// lookup finds the callable structure with the given method name
114-
func (r *RegMethodSet) lookup(method string) (*callable, bool) {
112+
func (r *regMethodSet) lookup(method string) (*callable, bool) {
115113
if c, ok := r.reg[method]; ok {
116114
return &c, true
117115
}
@@ -129,7 +127,7 @@ type callable struct {
129127
func (inst *Instance) RegisterMethods() {
130128
reg := make(map[string]callable)
131129
inst.registerOne("fsi", &FSIImpl{}, reg)
132-
inst.regMethods = &RegMethodSet{reg: reg}
130+
inst.regMethods = &regMethodSet{reg: reg}
133131
}
134132

135133
func (inst *Instance) registerOne(ourName string, impl interface{}, reg map[string]callable) {
@@ -139,51 +137,44 @@ func (inst *Instance) registerOne(ourName string, impl interface{}, reg map[stri
139137
for k := 0; k < num; k++ {
140138
m := implType.Method(k)
141139
lowerName := strings.ToLower(m.Name)
142-
funcName := ourName + "." + lowerName
140+
funcName := fmt.Sprintf("%s.%s", ourName, lowerName)
143141

144142
// Validate the parameters to the method
145143
// should have 3 input parameters: (receiver, scope, input struct)
146144
// should have 2 output parametres: (output value, error)
147145
// TODO(dustmop): allow variadic returns: error only, cursor for pagination
148146
f := m.Type
149147
if f.NumIn() != 3 {
150-
log.Errorf("%s: bad number of inputs: %d", funcName, f.NumIn())
151-
continue
148+
log.Fatalf("%s: bad number of inputs: %d", funcName, f.NumIn())
152149
}
153150
if f.NumOut() != 2 {
154-
log.Errorf("%s: bad number of outputs: %d", funcName, f.NumOut())
155-
continue
151+
log.Fatalf("%s: bad number of outputs: %d", funcName, f.NumOut())
156152
}
157153
// First input must be the receiver
158154
inType := f.In(0)
159155
if inType != implType {
160-
log.Errorf("%s: first input param should be impl, got %v", funcName, inType)
161-
continue
156+
log.Fatalf("%s: first input param should be impl, got %v", funcName, inType)
162157
}
163158
// Second input must be a scope
164159
inType = f.In(1)
165-
if inType.Name() != "Scope" {
166-
log.Errorf("%s: second input param should be scope, got %v", funcName, inType)
167-
continue
160+
if inType.Name() != "scope" {
161+
log.Fatalf("%s: second input param should be scope, got %v", funcName, inType)
168162
}
169163
// Third input is a pointer to the input struct
170164
inType = f.In(2)
171165
if inType.Kind() != reflect.Ptr {
172-
log.Errorf("%s: third input param must be a struct pointer, got %v", funcName, inType)
173-
continue
166+
log.Fatalf("%s: third input param must be a struct pointer, got %v", funcName, inType)
174167
}
175168
inType = inType.Elem()
176169
if inType.Kind() != reflect.Struct {
177-
log.Errorf("%s: third input param must be a struct pointer, got %v", funcName, inType)
178-
continue
170+
log.Fatalf("%s: third input param must be a struct pointer, got %v", funcName, inType)
179171
}
180172
// First output is anything
181173
outType := f.Out(0)
182174
// Second output must be an error
183175
outErrType := f.Out(1)
184176
if outErrType.Name() != "error" {
185-
log.Errorf("%s: second output param should be error, got %v", funcName, inType)
186-
continue
177+
log.Fatalf("%s: second output param should be error, got %v", funcName, outErrType)
187178
}
188179

189180
// Save the method to the registration table
@@ -193,12 +184,16 @@ func (inst *Instance) registerOne(ourName string, impl interface{}, reg map[stri
193184
InType: inType,
194185
OutType: outType,
195186
}
196-
log.Infof("%d: registered %s(*%s) %v", k, funcName, inType, outType)
187+
log.Debugf("%d: registered %s(*%s) %v", k, funcName, inType, outType)
197188
}
198189
}
199190

200191
// methodEndpoint returns a method name and returns the API endpoint for it
201192
func methodEndpoint(method string) APIEndpoint {
193+
// TODO(dustmop): This is here temporarily. /fsi/write/ works differently than
194+
// other methods; their http API endpoints are only their method name, for
195+
// exmaple /status/. This should be replaced with an explicit mapping from
196+
// method names to endpoints.
202197
if method == "fsi.write" {
203198
return "/fsi/write/"
204199
}

lib/fsi.go

Lines changed: 10 additions & 10 deletions
Original file line numberDiff line numberDiff line change
@@ -175,7 +175,7 @@ func (m *FSIMethods) EnsureRef(ctx context.Context, p *LinkParams) (*dsref.Versi
175175
type FSIImpl struct{}
176176

177177
// CreateLink creates a connection between a working drirectory and a dataset history
178-
func (*FSIImpl) CreateLink(scope Scope, p *LinkParams) (*dsref.VersionInfo, error) {
178+
func (*FSIImpl) CreateLink(scope scope, p *LinkParams) (*dsref.VersionInfo, error) {
179179
ctx := scope.Context()
180180

181181
ref, _, err := scope.ParseAndResolveRef(ctx, p.Refstr, "local")
@@ -189,7 +189,7 @@ func (*FSIImpl) CreateLink(scope Scope, p *LinkParams) (*dsref.VersionInfo, erro
189189
// Unlink removes the connection between a working directory and a dataset. If given only a
190190
// directory, will remove the link file from that directory. If given only a reference,
191191
// will remove the fsi path from that reference, and remove the link file from that fsi path
192-
func (*FSIImpl) Unlink(scope Scope, p *LinkParams) (string, error) {
192+
func (*FSIImpl) Unlink(scope scope, p *LinkParams) (string, error) {
193193
ctx := scope.Context()
194194

195195
if p.Dir != "" && p.Refstr != "" {
@@ -225,7 +225,7 @@ func (*FSIImpl) Unlink(scope Scope, p *LinkParams) (string, error) {
225225

226226
// Status checks for any modifications or errors in a linked directory against its previous
227227
// version in the repo. Must only be called if FSI is enabled for this dataset.
228-
func (*FSIImpl) Status(scope Scope, p *LinkParams) ([]StatusItem, error) {
228+
func (*FSIImpl) Status(scope scope, p *LinkParams) ([]StatusItem, error) {
229229
ctx := scope.Context()
230230

231231
if p.Dir == "" && p.Refstr == "" {
@@ -252,7 +252,7 @@ func (*FSIImpl) Status(scope Scope, p *LinkParams) ([]StatusItem, error) {
252252

253253
// WhatChanged gets changes that happened at a particular version in the history of the given
254254
// dataset reference.
255-
func (*FSIImpl) WhatChanged(scope Scope, p *LinkParams) ([]StatusItem, error) {
255+
func (*FSIImpl) WhatChanged(scope scope, p *LinkParams) ([]StatusItem, error) {
256256
ctx := scope.Context()
257257

258258
ref, _, err := scope.ParseAndResolveRef(ctx, p.Refstr, "local")
@@ -265,7 +265,7 @@ func (*FSIImpl) WhatChanged(scope Scope, p *LinkParams) ([]StatusItem, error) {
265265

266266
// Checkout method writes a dataset to a directory as individual files.
267267
// TODO(dustmop): Returned string is not used, remove it once dispatch is compatible
268-
func (*FSIImpl) Checkout(scope Scope, p *LinkParams) (string, error) {
268+
func (*FSIImpl) Checkout(scope scope, p *LinkParams) (string, error) {
269269
ctx := scope.Context()
270270

271271
// Require a non-empty, absolute path for the checkout
@@ -330,7 +330,7 @@ func (*FSIImpl) Checkout(scope Scope, p *LinkParams) (string, error) {
330330
}
331331

332332
// Write mutates a linked dataset on the filesystem
333-
func (*FSIImpl) Write(scope Scope, p *FSIWriteParams) ([]StatusItem, error) {
333+
func (*FSIImpl) Write(scope scope, p *FSIWriteParams) ([]StatusItem, error) {
334334
ctx := scope.Context()
335335

336336
if p.Ds == nil {
@@ -363,7 +363,7 @@ func (*FSIImpl) Write(scope Scope, p *FSIWriteParams) ([]StatusItem, error) {
363363

364364
// Restore method restores a component or all of the component files of a dataset from the repo
365365
// TODO(dustmop): Returned string is not used, remove it once dispatch is compatible
366-
func (*FSIImpl) Restore(scope Scope, p *RestoreParams) (string, error) {
366+
func (*FSIImpl) Restore(scope scope, p *RestoreParams) (string, error) {
367367
ctx := scope.Context()
368368

369369
ref, _, err := scope.ParseAndResolveRef(ctx, p.Refstr, "local")
@@ -428,7 +428,7 @@ func (*FSIImpl) Restore(scope Scope, p *RestoreParams) (string, error) {
428428
}
429429

430430
// Init creates a new dataset in a working directory
431-
func (*FSIImpl) Init(scope Scope, p *InitDatasetParams) (string, error) {
431+
func (*FSIImpl) Init(scope scope, p *InitDatasetParams) (string, error) {
432432
ctx := scope.Context()
433433

434434
if p.UseDscache {
@@ -440,14 +440,14 @@ func (*FSIImpl) Init(scope Scope, p *InitDatasetParams) (string, error) {
440440
}
441441

442442
// CanInitDatasetWorkDir returns nil if the directory can init a dataset, or an error if not
443-
func (*FSIImpl) CanInitDatasetWorkDir(scope Scope, p *InitDatasetParams) (bool, error) {
443+
func (*FSIImpl) CanInitDatasetWorkDir(scope scope, p *InitDatasetParams) (bool, error) {
444444
// TODO(dustmop): Change dispatch so that implementations can only return 1 value
445445
// if that value is an error
446446
return true, scope.FSISubsystem().CanInitDatasetWorkDir(p.TargetDir, p.BodyPath)
447447
}
448448

449449
// EnsureRef will modify the directory path in the repo for the given reference
450-
func (*FSIImpl) EnsureRef(scope Scope, p *LinkParams) (*dsref.VersionInfo, error) {
450+
func (*FSIImpl) EnsureRef(scope scope, p *LinkParams) (*dsref.VersionInfo, error) {
451451
ctx := scope.Context()
452452

453453
ref, err := dsref.Parse(p.Refstr)

lib/lib.go

Lines changed: 1 addition & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -701,7 +701,7 @@ type Instance struct {
701701
repoPath string
702702
cfg *config.Config
703703

704-
regMethods *RegMethodSet
704+
regMethods *regMethodSet
705705

706706
streams ioes.IOStreams
707707
repo repo.Repo

0 commit comments

Comments
 (0)