Conversation
|
some bug exists this PR cannot process this now 【FIXED】 |
bmancini55
left a comment
bmancini55
left a comment
There was a problem hiding this comment.
Thanks for submitting the issue and PR, much appreciated! Overall things look pretty clean, nicely done!
The two concerns in the client itself are why the BasicMultiClient is being used and that rate limiting isn't added to the client. From looking at the documentation, I think once rate limiting is added you can simplify the code and just use the BasicClient implementation instead of BasicMultiClient.
|
Thank you for advice. |
bmancini55
left a comment
bmancini55
left a comment
There was a problem hiding this comment.
@develoQ thanks for making the requested changes!
A few things still need to be fixed:
- you need to use the
_sendmethod where you usingthis._wss.sendthroughout the client - you need to override
_onClosingand clean up the rate limiter function. The contributing guide mentions it but it's not clear what that means.
If you could be so kind as to add some commenting for future developers at the class level (link to documentation for the API, what channels are supported, rate limiting for the exchange, etc) as well as sample JSON parsed in the _construct* methods.
Thanks!
| const Level2Point = require("../level2-point"); | ||
| const Level2Snapshot = require("../level2-snapshot"); | ||
| const moment = require("moment"); | ||
|
|
There was a problem hiding this comment.
Can you add a class level comment that points to the documentation and indicates that types of subscriptions that are available to the client?
bmancini55
left a comment
bmancini55
left a comment
There was a problem hiding this comment.
@develoQ, much thanks for making the changes! One minor fix and should be good to merge.
| } | ||
|
|
||
| _onClosing() { | ||
| this._sendMessage.cancel(); |
There was a problem hiding this comment.
This should be this._send.cancel(); since that's what your method is called.
2 participants
#246