-
Notifications
You must be signed in to change notification settings - Fork 1.2k
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
Add Support for Generic Handlers #1013
Conversation
Yeah no problem. I can help convert the tests too as part of the pr if
you'd like. Unfortunately I have a ton of down time at the moment. 😉
…On Tue, Apr 16, 2024, 6:46 AM Jimmy Bogard ***@***.***> wrote:
***@***.**** requested changes on this pull request.
------------------------------
In test/MediatR.Tests/SendTests.cs
<#1013 (comment)>:
> @@ -213,4 +251,59 @@ public async Task Should_raise_execption_on_null_request()
await Should.ThrowAsync<ArgumentNullException>(async () => await mediator.Send(default!));
}
+
+ [Fact]
Nitpick, but these tests don't exercise those service scanning tests.
You'd want to use services.AddMediatR instead. Which I am fine with, I
need to convert all the tests to stop using Lamar anyway.
—
Reply to this email directly, view it on GitHub
<#1013 (review)>,
or unsubscribe
<https://github.com/notifications/unsubscribe-auth/ACL2QUWEXFEUWRUH75UCXLDY5UTRPAVCNFSM6AAAAABFEKKOYCVHI2DSMVQWIX3LMV43YUDVNRWFEZLROVSXG5CSMV3GSZLXHMZDAMBTG4YDGOBVGM>
.
You are receiving this because you authored the thread.Message ID:
***@***.***>
|
Looks like a neat, new feature. I'd like to see additional documentation with more practical examples, but I think this is neat. |
@jbogard , are there any more changes necessary for this I think I converted the tests to use the default .net container and removed the dependency on Lamar like you suggested. Just wondering if anything else was needed. Thanks. |
@zachpainter77 if you have time, can you add an example in the wiki? |
Yes no problem will do ASAP. 😊
…On Thu, Jun 6, 2024, 1:03 PM Jimmy Bogard ***@***.***> wrote:
@zachpainter77 <https://github.com/zachpainter77> if you have time, can
you add an example in the wiki?
—
Reply to this email directly, view it on GitHub
<#1013 (comment)>,
or unsubscribe
<https://github.com/notifications/unsubscribe-auth/ACL2QUSGQI7IB6I7SEWYMY3ZGC6A5AVCNFSM6AAAAABFEKKOYCVHI2DSMVQWIX3LMV43OSLTON2WKQ3PNVWWK3TUHMZDCNJTGMYTINBZGM>
.
You are receiving this because you were mentioned.Message ID:
***@***.***>
|
[![Mend Renovate](https://app.renovatebot.com/images/banner.svg)](https://renovatebot.com) This PR contains the following updates: | Package | Change | Age | Adoption | Passing | Confidence | |---|---|---|---|---|---| | [MediatR](https://togithub.com/jbogard/MediatR) | `12.2.0` -> `12.3.0` | [![age](https://developer.mend.io/api/mc/badges/age/nuget/MediatR/12.3.0?slim=true)](https://docs.renovatebot.com/merge-confidence/) | [![adoption](https://developer.mend.io/api/mc/badges/adoption/nuget/MediatR/12.3.0?slim=true)](https://docs.renovatebot.com/merge-confidence/) | [![passing](https://developer.mend.io/api/mc/badges/compatibility/nuget/MediatR/12.2.0/12.3.0?slim=true)](https://docs.renovatebot.com/merge-confidence/) | [![confidence](https://developer.mend.io/api/mc/badges/confidence/nuget/MediatR/12.2.0/12.3.0?slim=true)](https://docs.renovatebot.com/merge-confidence/) | --- ### Release Notes <details> <summary>jbogard/MediatR (MediatR)</summary> ### [`v12.3.0`](https://togithub.com/jbogard/MediatR/releases/tag/v12.3.0) #### What's Changed - Fix AutoRegisterRequestProcessors to include all implementations by [@​hisuwh](https://togithub.com/hisuwh) in [https://github.com/jbogard/MediatR/pull/989](https://togithub.com/jbogard/MediatR/pull/989) - [#​1016](https://togithub.com/jbogard/MediatR/issues/1016) Use repo readme for base package by [@​thompson-tomo](https://togithub.com/thompson-tomo) in [https://github.com/jbogard/MediatR/pull/1017](https://togithub.com/jbogard/MediatR/pull/1017) - Add Support for Generic Handlers by [@​zachpainter77](https://togithub.com/zachpainter77) in [https://github.com/jbogard/MediatR/pull/1013](https://togithub.com/jbogard/MediatR/pull/1013) #### New Contributors - [@​hisuwh](https://togithub.com/hisuwh) made their first contribution in [https://github.com/jbogard/MediatR/pull/989](https://togithub.com/jbogard/MediatR/pull/989) - [@​thompson-tomo](https://togithub.com/thompson-tomo) made their first contribution in [https://github.com/jbogard/MediatR/pull/1017](https://togithub.com/jbogard/MediatR/pull/1017) - [@​zachpainter77](https://togithub.com/zachpainter77) made their first contribution in [https://github.com/jbogard/MediatR/pull/1013](https://togithub.com/jbogard/MediatR/pull/1013) **Full Changelog**: jbogard/MediatR@v12.2.0...v12.3.0 </details> --- ### Configuration 📅 **Schedule**: Branch creation - "before 7am on Sunday,before 7am on Wednesday" (UTC), Automerge - At any time (no schedule defined). 🚦 **Automerge**: Disabled by config. Please merge this manually once you are satisfied. ♻ **Rebasing**: Whenever PR becomes conflicted, or you tick the rebase/retry checkbox. 🔕 **Ignore**: Close this PR and you won't be reminded about this update again. --- - [ ] <!-- rebase-check -->If you want to rebase/retry this PR, check this box --- This PR has been generated by [Mend Renovate](https://www.mend.io/free-developer-tools/renovate/). View repository job log [here](https://developer.mend.io/github/digdir/dialogporten). <!--renovate-debug:eyJjcmVhdGVkSW5WZXIiOiIzNy4zOTMuMCIsInVwZGF0ZWRJblZlciI6IjM3LjM5My4wIiwidGFyZ2V0QnJhbmNoIjoibWFpbiIsImxhYmVscyI6W119--> Co-authored-by: renovate[bot] <29139614+renovate[bot]@users.noreply.github.com> Co-authored-by: Ole Jørgen Skogstad <skogstad@softis.net>
Unfortunately, this assumption is incorrect. I'm getting an error on startup with the latest version: It would seem this is incompatible with some code I already have for handling Open Generic Handlers.
This is a slight misnomer. You DI tool likely has a way to handle this - which is what I'm doing, using a fallback policy to catch missing open handlers and close them. I think the main problem is you are only handling a single generic argument here: https://github.com/jbogard/MediatR/pull/1013/files#diff-c4d1d5960b86a86bee0fd5974803c90f8656cde09914a4c43ae14dc193c3cf35R195 But my handlers have multiple. |
Yes, you are absolutely correct.. I found out about this a while back (#1038 ) and have been working on a fix but haven't had time until today. So I have pushed a pull request out that fixes it and adds some extensive testing.. But there could be more to discuss. Can you please review it if you have time? PR -> #1048 |
I'm away from my computer today but I'll try and review it in the next few days. |
Any updates here? Upgrading to this version breaks the project. |
There is a PR to fix the breaking change.. here: #1048 |
This PR essentially adds the required logic to register open generic request handlers.
For example:
Huge fan of this library.. With that said.. I find it kind of tedious to have to create separate handlers for each thing, when the code is pretty much copy paste. Just the entity or type I'm working with has changed. I think it would be much easier to work with if we could essentially cut some corners by leveraging c# generics.
The only way you can do this without needing a third party library is use assembly scanning and register every single concrete implementation of the generic handlers. So that is what the code is doing.
I also added some tests to ensure the handlers were being registered.
**Note: this feature doesn't introduce any breaking changes. The feature is simply opt in by creating handlers like above.. if you don't want to use them then you can keep on using the library the same way without any hiccups.