-
-
Notifications
You must be signed in to change notification settings - Fork 25
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
Improve customization of service registration #17
Conversation
Hi @Thomas-X In the meantime, I would appreciate if you could revert some codestyle changes that slipped through when you committed your changes. I'm asking this because it makes it easier to understand what was changed in the PR. |
Changed some of the formatting that slipped through, maybe it could be worth adding a .editorconfig for the future |
Hi @Thomas-X, sorry for the delay. I have been thinking a bit about this PR and I think that adding (yet) another parameter to the registration methods is not sustainable in the long term. I'm considering the idea of using a configuration delegate that can be used to customize the registered services as needed. Something like below services.UseNotificationHandler(c => c.UseSerializer<MyCustomSerializer>().EnableParallelExecution()); So my plan is to rewrite this PR with this design but I am not sure when I have time to do so. |
Hi @Kralizek, no problem, we all have lives :). I think that using a configuration delegate definitely makes it more future-proof and more extensible, so we should rewrite this PR for that. I'm a bit dry on time as well, but I can take a look at the idea next week(end) probably, and see if I'm able to implement it! (I'm a junior dev). Unless you beat me to the punch, of course |
@Thomas-X managed to squeeze a nick of time for the lib part. tests need to be updated as the build is failing now. |
@Kralizek That's really awesome, I'll see what I can do soon to get it built and the tests green |
Could you add an implementation of |
Left to do before merge:
Left to do before new release:
|
The idea for this PR is so you can pass your own serialization logic incase your incoming messages are in an other format than JSON.
I added unit tests too, but let me know if there's any questions or remarks on how it's structured. README is updated to reflect the (optional) API changes as well