Misc. fixes #852
Misc. fixes #852
Conversation
| @@ -54,9 +54,7 @@ func (s *PLCServer) GetDocument(ctx context.Context, didstr string) (*did.Docume | |||
| return &doc, nil | |||
| } | |||
|
|
|||
| func (s *PLCServer) FlushCacheFor(did string) { | |||
| return | |||
atproto/lexicon/cmd/lextool/net.go
Outdated
| "github.com/bluesky-social/indigo/atproto/data" | ||
| "github.com/bluesky-social/indigo/atproto/identity" | ||
| "github.com/bluesky-social/indigo/atproto/lexicon" | ||
| "github.com/bluesky-social/indigo/atproto/syntax" | ||
|
|
||
| "github.com/bobg/errors" |
There was a problem hiding this comment.
A humble suggestion. Please see https://github.com/bobg/errors#readme
| @@ -29,12 +28,12 @@ func runValidateRecord(cctx *cli.Context) error { | |||
| cat := lexicon.NewBaseCatalog() | |||
| err := cat.LoadDirectory(p) | |||
| if err != nil { | |||
| return err | |||
| return errors.Wrap(err, "in LoadDirectory") | |||
There was a problem hiding this comment.
Especially when a function has multiple error return sites, it's useful to wrap them to know which site did the returning.
| } | ||
|
|
||
| body, err := data.UnmarshalJSON(respBytes) | ||
| if err != nil { |
automod/flagstore/flagstore_mem.go
Outdated
| for _, f := range flags { | ||
| v = append(v, f) | ||
| } | ||
| v, _ := s.Data[key] |
There was a problem hiding this comment.
If key is not in s.Data, then v will be []string(nil), which is suitable for using directly in the append that follows. All the other stuff is extraneous.
| // TODO: do we still just do this if it errors? | ||
| c.complete <- job.act.Uid | ||
| // TODO: plumb a context object into this function and use select{} to multiplex its Done channel with c.repoSync | ||
| for job := range c.repoSync { |
There was a problem hiding this comment.
More straightforward than a for { select { ... } } with a single case.
| "github.com/multiformats/go-multihash" | ||
| mh "github.com/multiformats/go-multihash" |
| ctxAuth struct{} | ||
| ctxAuthScope struct{} | ||
| ctxDID struct{} | ||
| ctxToken struct{} | ||
| ctxUser struct{} |
There was a problem hiding this comment.
Use distinct types as keys for context.WithValue, not strings, to avoid collisions with other packages doing the same thing.
This file encapsulates setting and getting of the particular context values needed by this package.
|
|
||
| user, ok := c.Get("user").(*gojwt.Token) | ||
| if !ok { | ||
| token := getToken(ctx) |
There was a problem hiding this comment.
This needs double-checking. The original code was confusing the Token types from two different jwt modules, and possibly confusing users with tokens.
| } | ||
| defer resp.Body.Close() |
There was a problem hiding this comment.
Required after any http.Get (and a few other operations).
| } | ||
| defer resp.Body.Close() |
| return err | ||
| var ( | ||
| oplog []map[string]any | ||
| dec = json.NewDecoder(resp.Body) |
There was a problem hiding this comment.
No need to read the whole response body into memory first - stream it straight to the JSON decoder.
| @@ -347,7 +342,7 @@ func (bgs *BGS) StartWithListener(listen net.Listener) error { | |||
| sendHeader = false | |||
| } | |||
|
|
|||
| bgs.log.Warn("HANDLER ERROR: (%s) %s", ctx.Path(), err) | |||
| bgs.log.Warn("HANDLER ERROR", "path", ctx.Path(), "err", err) | |||
There was a problem hiding this comment.
Structured (not formatted-string) logging, here and in a couple of other spots below too.
|
Thanks! I think what will probably happen here is that i'll cherry-pick a subset of these when we get a chance. Keeping small/discrete commits is helpful. We probably won't pull in new dependencies (eg, your errors package). I should really get staticcheck wired up to our regular linting. |
|
Makes sense, thanks @bnewbold. I'm interested and able to help - please let me know whether and how you'd like me to. (E.g., I could split this PR into multiple smaller ones - by directory, say, or by type-of-change.) |
Are contributions like this welcome?
This PR contains numerous fixes to problems identified by staticcheck, plus a few other style, correctness, and best-practice items.
Several unused functions, variables, and struct fields are removed. In some places, imports are rearranged according to goimports with
-local github.com/bluesky-social/indigo. Some unnecessary import aliases are removed.Where it's not self-explanatory, I've annotated the different types of change inline to describe what's going on.