jbogard / MediatR.Extensions.Microsoft.DependencyInjection

MediatR extensions for Microsoft.Extensions.DependencyInjection
MIT License
327 stars 89 forks source link

Derivingrequests #33

Closed Condra963 closed 6 years ago

Condra963 commented 6 years ago

IRequest should match the correct handler. Necessary when you derive from an IRequest.

Condra963 commented 6 years ago

I have a question, would you approve that we can subscribe on Slack to receive notifications for your repositories? You can do it with this link: https://github.com/settings/installations/185846

jbogard commented 6 years ago

404?

On Tue, May 22, 2018 at 7:05 PM Condra963 notifications@github.com wrote:

I have a question, would you approve that we can subscribe on Slack to receive notifications for your repositories? You can do it with this link: https://github.com/settings/installations/185846

— You are receiving this because you are subscribed to this thread. Reply to this email directly, view it on GitHub https://github.com/jbogard/MediatR.Extensions.Microsoft.DependencyInjection/pull/33#issuecomment-391086611, or mute the thread https://github.com/notifications/unsubscribe-auth/AAGYMjlv3Vm5KruFT4jdpSkjxY266gN-ks5t1FOFgaJpZM4UCrmd .

Condra963 commented 6 years ago

Slack telled me to gave you the previous URL. Maybe these URL's helps you more? https://slack.github.com/ https://get.slack.help/hc/en-us/articles/232289568-GitHub-for-Slack

jbogard commented 6 years ago

What slack channel am I supposed to link to? I don't have one for MediatR.

On Tue, May 22, 2018 at 3:43 PM, Condra963 notifications@github.com wrote:

Slack telled me to gave you the previous URL. Maybe these URL's helps you more? https://slack.github.com/ https://get.slack.help/hc/en-us/articles/232289568-GitHub-for-Slack

— You are receiving this because you commented. Reply to this email directly, view it on GitHub https://github.com/jbogard/MediatR.Extensions.Microsoft.DependencyInjection/pull/33#issuecomment-391134166, or mute the thread https://github.com/notifications/unsubscribe-auth/AAGYMpcyzXI8_DRwsfQxRjWDz1vwHgCVks5t1HiFgaJpZM4UCrmd .

Condra963 commented 6 years ago

You don't have to link to a slack channel, you just have to allow github that people can subscribe with slack. You can choose which respositories they can subscribe to with slack. click this link. That makes it possible for people to subscribe a channel of there own to your respositories.

And i am just wondering, do you have any remarks on my pull-request? Do you aggree with my modifications or not?

jbogard commented 6 years ago

What's the use case here?

Condra963 commented 6 years ago

I want it to be possible to use deriving requests. What i mean with that:

I have for example PagingQuery<Foo> that calls the handler GetFoosQueryHandler. This query has some parameters.

Now i want another query ExtendingGetFoosQuery : PagingQuery<Foo> that has some extra properties above the base request. And with that one i want to call ExtendingGetFoosQueryHandler.

In the current registration of the IRequest this does not work. Or it works some times because it's registring in some order. My fix makes this possible, i also integrated a test that shows you what it does. If you disable my modification and run the test you will see it's not working as it should. It will register the wrong Handler.

arvdrpoo commented 6 years ago

I also stumbled on this issue and I was wondering if this functionality will possibly be implemented or not? Or do you have a different idea how we should handle this use-case?

jbogard commented 6 years ago

I still don't understand the use cases - why would you have two handlers? Is the first a general handler, and the second one more specific?

Personally, I'd rather support this by you explicitly registering the handler, instead of my library making some assumptions.

arvdrpoo commented 6 years ago

Exactly, I have one general handler and one more specific one.
I'll try to give an example:

Let's say my model consists of a list of Person and in 95% of the cases I want to add a Person to the end of the list.
I would have a AddPersonToListCommand having one property: Person Person, and a AddPersonToListCommandHandler that would take care of adding the Person to the end of the list.

In the other 5% of the cases I want to add a Person at the end of the list with a certain offset, creating gaps in the list.
Now I could add a int? Offset property to the existing AddPersonToListCommand and have a check for the Offset value in the AddPersonToListCommandHandler (but this doesn't feel quite right).
Or, I could add AddPersonToListAtOffsetCommand with properties: int Offset and Person Person; and a AddPersonToListAtOffsetCommandHandler that would deal with this command.

Since AddPersonToListCommand and AddPersonToListAtOffsetCommand are closely related and share a common field, I would like to have AddPersonToListAtOffsetCommand derive from AddPersonToListCommand so it would inherit the shared Person property and only needs to define the extra Offset property.

jbogard commented 6 years ago

Since this would be a breaking behavioral change, is there a reason why someone wouldn't want this behavior, or want to opt-out?

arvdrpoo commented 6 years ago

I'm not really sure if it is a breaking change, more a fix for the following.
If I add the two commands and two handlers as described, only the first handler found during resolution of the command (I think (or the handler of the base command)) will be called when the derived command is sent.

I don't see a reason at the moment why someone would want to opt out.
There are two cases: