Open shaynevanasperen opened 2 years ago
Hey @shaynevanasperen ,
thanks for the PR. Could you please split it into multiple separate PRs?
5000+ lines are definitely too much for a proper review.
A few high-level points:
ILogger
. Also, it is important to start with Websocket.Client
and then replicate it to dependent libraries.Subject
. The current code is implemented this way on purpose, hiding Subjects
and only exposing IObservables
. So the class API is self-describing and cannot be used wrongly. We should probably not to expose Subjects
directly, but make it more explicit. For example, we can provide methods StreamFakeData(xxx)
, very similar to Websocket.Client StreamFakeMessage().
I've exposed the underlying
Subject<T>
members in bothxxStreams
classes. This is because I want to be able to stream fake messages for testing my program which uses this library. Although that was already possible by using fakeIWebsocketClient
objects, that method is very clunky and requires that I send raw text instead of just POCO objects. There is no good reason for hiding theSubject<T>
members behindIObservable<T>
, so the most straight-forward way to enable easy testing is to just expose theSubject<T>
members directly.Summary of changes:
PrivateTrade
classIBitfinexPublicWebsocketClient
andIBitfinexAuthenticatedWebsocketClient
interfacesIBitfinexCommunicator
interface and implementation classLibLog
package reference withMicrosoft.Extensions.Logging.Abstractions
(asLigLog
is marked as deprecated)