Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Update streams objects to publicly expose underlying Subjects #44

Open
wants to merge 1 commit into
base: master
Choose a base branch
from

Conversation

shaynevanasperen
Copy link

@shaynevanasperen shaynevanasperen commented Dec 11, 2021

I've exposed the underlying Subject<T> members in both xxStreams 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 fake IWebsocketClient 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 the Subject<T> members behind IObservable<T>, so the most straight-forward way to enable easy testing is to just expose the Subject<T> members directly.

Summary of changes:

  • Separate clients for public/authenticated streams (as speficied in the Bitfinex API documentation)
  • Add missing functionality for Funding Offers, Funding Credits and Funding Loans
  • Add missing functionality for private Funding trades
  • Add missing Client Id property in PrivateTrade class
  • Introduce new IBitfinexPublicWebsocketClient and IBitfinexAuthenticatedWebsocketClient interfaces
  • Remove superfluous IBitfinexCommunicator interface and implementation class
  • Refactor to use file-scoped namespaces
  • Remove custom validation utility (so that exception stack traces are raised from the method itself rather than the ulitity method)
  • Replace LibLog package reference with Microsoft.Extensions.Logging.Abstractions (as LigLog is marked as deprecated)
  • Update other NuGet packages to the latest versions

… of just IObservables (to facilitate testing in consuming code by sending fake messages)

* Separate clients for public/authenticated streams (as speficied in the Bitfinex API documentation)
* Add missing functionality for Funding Offers, Funding Credits and Funding Loans
* Add missing functionality for private Funding trades
* Add missing Client Id property in PrivateTrade class
* Introduce new IBitfinexPublicWebsocketClient and IBitfinexAuthenticatedWebsocketClient interfaces
* Remove superfluous IBitfinexCommunicator interface and implementation class
* Refactor to use file-scoped namespaces
* Remove custom validation utility (so that exception stack traces are raised from the method itself rather than the ulitity method)
* Replace LibLog package reference with Microsoft.Extensions.Logging.Abstractions (as LigLog is marked as deprecated)
* Update other NuGet packages to the latest versions
<PackageId>Bitfinex.Client.Websocket</PackageId>
<Version>4.0.0</Version>
<Version>5.0.0-beta.7</Version>
Copy link
Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

You'll want to update this to remove the prerelease suffix.

@Marfusios
Copy link
Owner

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:

  • Replacing LibLog is a good idea, but we need to discuss how to do it correctly. Not to break existing clients, but still provide possibility to set ILogger. Also, it is important to start with Websocket.Client and then replicate it to dependent libraries.
  • I understand your requirement about 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().

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants