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

Code adaptation #5

Open
tinohager opened this issue Aug 20, 2024 · 3 comments
Open

Code adaptation #5

tinohager opened this issue Aug 20, 2024 · 3 comments

Comments

@tinohager
Copy link
Contributor

Would you accept a pull request with which I bring more structure to the project?

  • I would rename the private variables to _clientWebSocket instead of private ClientWebSocket WebSocket.
  • I would add an ILogger
  • I would integrate CancellationTokens
  • I would change the method names to StartAsync.

That would of course be a breaking change...

@akacdev
Copy link
Owner

akacdev commented Aug 20, 2024

Since I initially released this library over two years ago, I’m sure the code could benefit from the best practices you've suggested. Feel free to proceed with those cosmetic changes and I'll be happy to merge them in.

@tinohager
Copy link
Contributor Author

@akacdev You are now welcome to take a look at my changes and try out the new version for yourself

@akacdev akacdev reopened this Aug 24, 2024
@akacdev
Copy link
Owner

akacdev commented Aug 24, 2024

There is a minor issue where the fire-and-forget method call in StartAsync swallows the potential CertstreamException from the message receiving loop.

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

No branches or pull requests

2 participants