search

karalabe / go-bluesky

Bluesky API client from Go
BSD 3-Clause "New" or "Revised" License
68 stars 7 forks source link

add some missing contexts on Dial, Login #1

Closed bradfitz closed 1 year ago

bradfitz commented 1 year ago

(If it does network operations or users might want timeouts or cancelation, it needs a context.)

karalabe commented 1 year ago

LGTM, but out of curiosity, do you think it might make sense to have out-of-the-box methods that hide the context, of should those always be in the APIs?

karalabe commented 1 year ago

Oh, and a more contributing style question, do you mind me pushing tiny nitpick fixes on top of a PR before merging? We usually do that when it just feels like a pointless back and forth to pinpoint something trivial only to wait half a world RTT to have it fixed before merging.

bradfitz commented 1 year ago

Go for it.

As for context hiding: I'm not sure the doubled API bloat would be worth it. Also I'm so used to seeing context whenever there will be something slow or hitting the network that hiding it would weird me out at this point. That said, I'm not exactly sure what API you have in mind.

karalabe commented 1 year ago

I guess we should also expose the context to CustomCall?

karalabe commented 1 year ago

Though if you have a better API for that, feel free to suggest. It was just a wonky first-thought attempt to allow doing custom atproto calls. Haven't thought much about it thb.

karalabe commented 1 year ago

Merged it as is as I've figured the CustomCall allows you to use your own context from the outside within the closure so it's fine as is. That method should be a wildcard anyway so no problem if it's a bit wonky.