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

Split IMediator interface in IPublisher and ISender interface #563

Merged
merged 8 commits into from Sep 28, 2020
Merged

Split IMediator interface in IPublisher and ISender interface #563

merged 8 commits into from Sep 28, 2020

Conversation

KenBonny
Copy link
Sponsor

The reason for this change is the quick twitter chat I had with Jimmy about this.

@KenBonny
Copy link
Sponsor Author

Oh yes, I've removed unused usings as this is a pet peeve of mine. 😃

Comment on lines 1 to 11
using System.Collections.Generic;
using System.Threading;
using System.Threading.Tasks;
using Shouldly;
using StructureMap;
using Xunit;

namespace MediatR.Tests
{
using System;
using System.Collections.Generic;
using System.Threading.Tasks;
using Shouldly;
using StructureMap;
using Xunit;

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I understand removing unused references, but I don't know the implications of moving usings from inside the namespace to outside, any reason for doing so?

Copy link
Sponsor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

There is no benefit to using a using statement inside or outside of the namespace. I think the only difference is the scope. So the usings inside the namespace are only accessible inside the namespace and the usings outside are for the whole file. Since there is only one namespace in this file, it won't matter.

When I saw this in the file, my neurotic nature wanted to put them together as they belong together.

Copy link
Sponsor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

If you want, I can go over the code and put them all together and order them alphabetically.

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

If they are functionally the same it doesn't matter to me, I'll defer to Jimmy. I've always put them outside but have seen them inside namespaces as well.

Copy link
Sponsor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Ok, I'll leave it up to @jbogard to decide if I roll this back, if I format and order the rest of the usings in the codebase or if this is ok.

Copy link
Owner

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I prefer consistency tbqh

Copy link
Sponsor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Would you like me to roll it back or would you like me to look through the codebase and format the other places?

Copy link
Owner

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

If it's not consistent in the codebase today, leave it alone.

Copy link
Sponsor Author

@KenBonny KenBonny Sep 27, 2020

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Should be rolled back now. I did remove the unused using, which I did throughout the codebase.

@lilasquared
Copy link
Contributor

@KenBonny once this is merged the dependency injection package should be updated as well to register the publisher and sender. https://github.com/jbogard/MediatR.Extensions.Microsoft.DependencyInjection

@KenBonny
Copy link
Sponsor Author

@lilasquared Thanks for the heads up. I think the only thing that needs to happen is for the MediatR package to be updated. If the IMediator interface is registered, then I do believe most functionality should work. I did see that there is a line that registers all types in an assembly instead of registering everything by hand. I think that takes care of registering every type and interface in the assembly.

@lilasquared
Copy link
Contributor

I'm pretty sure that only scans and searches for implementations of the handlers. I'm referring to https://github.com/jbogard/MediatR.Extensions.Microsoft.DependencyInjection/blob/master/src/MediatR.Extensions.Microsoft.DependencyInjection/Registration/ServiceRegistrar.cs#L220
where the IMediator type is registered explicitly. If you test your branch with that package and you try to inject ISender it fails to resolve.

@@ -2,7 +2,6 @@

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I kinda feel that this whole file should be reverted, bar the removing of the namespace - just inconsistent formatting

Copy link
Sponsor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I rolled it back. On save, my IDE (Rider) formats all open files to my coding standards.

Copy link
Owner

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

So basically I need an .editorconfig

@KenBonny
Copy link
Sponsor Author

Is this ok, can it be merged or do other steps need to happen? That the other reviewers approve, for example. Or do I need to press a button to merge the PR?

@jbogard jbogard merged commit 168a433 into jbogard:master Sep 28, 2020
@KenBonny
Copy link
Sponsor Author

KenBonny commented Oct 4, 2020

Don't mistake my question for impatience, but when will this be available in the package?

I'm asking so I can also update the DI package @lilasquared mentioned. I have a leaky memory (I should get that looked after) and if this takes a month or 2 (which is fine 😄 ), then I need to add a calendar notification to remind me to do the DI update then.

@jbogard
Copy link
Owner

jbogard commented Oct 7, 2020

Tagged! Should be on NuGet shortly.

@davidroth
Copy link

davidroth commented Oct 8, 2020

@jbogard If you make such changes, please consider increasing the major version instead of the minor version in the future ;-)

This interface split is technically a breaking change.
For example if an assembly "A" references IMediator from 8.0.2, but at runtime 8.2 is supplied (assembly unification at build time), the interface split leads to the following runtime exception in "A" if it has not been recompiled with 8.2.

System.MissingMethodException : Method not found: 'System.Threading.Tasks.Task`1<!!0> MediatR.IMediator.Send(MediatR.IRequest`1<!!0>, System.Threading.CancellationToken)'.

Thanks 😃

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

Successfully merging this pull request may close these issues.

None yet

5 participants