-
Notifications
You must be signed in to change notification settings - Fork 20
Feat cancellation token on IMessageHandler #104
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
Conversation
|
Perhaps we can make some beta release first and have your team test it out? Even though I see that the change makes sense, I am a little afraid of just going ahead with it and breaking dafda for everyone. On one hand, we are still somewhere in v0.1 world so breaking changes should be expected, but on the other, dafda api has been stable for years now… Impediments before a stable release can be made:
Btw, the software committee (new dafda owners) has just started a talk towards dafda v1.0 roadmap - there will be possibly some more breaking changes released with version 1, and this could be one of them. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
there is a script for updating the version, we will do it after the PR is merged
|
I released this as beta 0.13.0-beta1, so you can try it out in your team. Did you have a time to go through the documentation? |
Hey, I have been using 0.13.0-beta1 for a few days now, and it's working as intended. I'd say the forced default on CancellationToken is not really needed. We need to anyway update all the consumers to implement CancellationToken. I will push a new commit as soon as I can ! |
|
| Status | Scanner | Total (0) | ||||
|---|---|---|---|---|---|---|
| ✅ | Code Security | 0 | 0 | 0 | 0 | 0 issues |
| Open Source Security | 0 | 0 | 0 | 0 | See details | |
| Licenses | 0 | 0 | 0 | 0 | See details |
💻 Catch issues earlier using the plugins for VS Code, JetBrains IDEs, Visual Studio, and Eclipse.
This reverts commit c0500d8.
Proposed solution changes interface
IMessageHandler<T>from:
to
As mentioned by @toizo here: 102 this interface is a core component, and users will have to update their implementation (even though is set as default)
Another approach would be to have another interface (fx
IAsyncMessageHandler) instead of changingIMessageHandler, but that will introduce its own complexities and doesn't really align with an async approach when working with tasks IMO.I would suggest to have it as a breaking change on a future release.