add some missing contexts on Dial, Login #1
Conversation
(If it does network operations or users might want timeouts or cancelation, it needs a context.)
|
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? |
|
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. |
|
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. |
|
I guess we should also expose the context to |
|
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. |
|
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. |
(If it does network operations or users might want timeouts or cancelation, it needs a context.)