CoinbasePro: Add subscription templating #3
Conversation
gbjk
force-pushed
the
feature/coinbase_sub_conf_v2
branch
from
0704970 to
9205394
Compare
| } | ||
| time.Sleep(time.Millisecond * 10) | ||
| errs = common.AppendError(errs, err) | ||
| } | ||
| return nil |
There was a problem hiding this comment.
Shouldn't errs be returned around here?
There was a problem hiding this comment.
No, We want to collect all of the errors, not just return on the very first one 😄
In reality, in situations where engine is running, we expect subs to be len(1).
However the for loop protects against a manual call to Subscribe before calling ExpandTemplates.
Also just cleaner than having to len protect subs and then use s := subs[0].
There was a problem hiding this comment.
I mean at the end of the function, on line 375, we should return errs instead of nil. If we haven't collected any errors, errs will just be nil anyway, and at the end it won't interrupt execution.
There was a problem hiding this comment.
😳
Sorry, you're completely right and I missed your point entirely
Erm. Yeah. Shocking I even brought this up to look at it 🤦
Fixed 62c48af
| } | ||
| time.Sleep(time.Millisecond * 10) | ||
| errs = common.AppendError(errs, err) | ||
| } | ||
| return nil |
There was a problem hiding this comment.
I mean at the end of the function, on line 375, we should return errs instead of nil. If we haven't collected any errors, errs will just be nil anyway, and at the end it won't interrupt execution.
Hello @cranktakular 👋
This PR is for merging into your existing coinbase api revamp branch.
ticker_batchsubusersubAny questions, just ask, here or on slack 🙂