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

Increasing level of extensibility #4094

Closed
wants to merge 2 commits into from
Closed

Increasing level of extensibility #4094

wants to merge 2 commits into from

Conversation

zorgoz
Copy link

@zorgoz zorgoz commented Apr 4, 2018

  • I have made ReflectedHubDescriptorProvider.BuildHubsCache method virtual. This way one can extend the default provider in an easy way, without the need to recreate all its functionality.
  • Because HubTypeExtensions were internal, one would have had to recreate that too. This is why I have made the latter public - if extensions like the one below are not enough.
  • Some minor maintainability related changes

Solves
#4077 These changes provide a solution for this issue by supporting the creation of such providers:

public class FilteredHubDescriptorProvider : ReflectedHubDescriptorProvider
{
	private readonly Predicate<HubDescriptor> _filter;

	public FilteredHubDescriptorProvider(IDependencyResolver resolver, Predicate<HubDescriptor> filter): base(resolver)
	{
		_filter = filter;
	}

	protected override IDictionary<string, HubDescriptor> BuildHubsCache()
	{
		return base.BuildHubsCache().Where(d => _filter(d.Value)).ToDictionary(x => x.Key, x=> x.Value);
	}
}

@dnfclas
Copy link

dnfclas commented Apr 4, 2018

CLA assistant check
All CLA requirements met.

Copy link
Contributor

@analogrelay analogrelay left a comment

Choose a reason for hiding this comment

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

Thanks for the contribution! Sorry about the delay in reviewing. You might have see from our blog that things got a little backlogged here.

I have some comments, but otherwise I think we're OK to take this change. If you can address that feedback I think we can get this merged in for the 2.4.0 release.

@@ -5,9 +5,9 @@

namespace Microsoft.AspNet.SignalR.Hubs
{
internal static class HubTypeExtensions
Copy link
Contributor

Choose a reason for hiding this comment

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

We can't really make these public as extension methods on Type. Unless there's an extremely good reason, we try to avoid attaching public extension methods on public BCL types like this because they appear for everyone using the type if they happen to have our namespace imported. (If we were writing these helpers now, we wouldn't have made them extension methods at all, even though they are internal 😉).

Can you either leave these as internal (and just copy the code for your own extensions) or convert them to standard static methods (which would require changing all the call-sites unfortunately).

Copy link
Author

Choose a reason for hiding this comment

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

Git your point. It is not that important after all. I have discarded this change during rebase.

}
}
}
public class ReflectedHubDescriptorProvider : IHubDescriptorProvider
Copy link
Contributor

Choose a reason for hiding this comment

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

For future reference, please avoid major reformatting like this in your contributions. We have to reformat it back to our standard code style in order to merge it :). Don't worry about it for this change, I can review the actual functional change just fine and resolve the formatting when I merge, but just for the future :).

Copy link
Author

Choose a reason for hiding this comment

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

I have noticed that only after pushed the changes. I have reworked this during rebase.

@analogrelay
Copy link
Contributor

Also, could you rebase your change on top of the latest copy of the master branch and then change the base branch of the PR in GitHub (after pushing your changes). To change the base branch of the PR, just click "Edit" in the top-right and change the branch from the drop down:

image

@analogrelay analogrelay added this to the 2.4.0 milestone Sep 24, 2018
…HubsCache method virtual, and HubTypeExpensions public. Provides one way of solving issue #4077.
@zorgoz
Copy link
Author

zorgoz commented Sep 25, 2018

Well, now CI build fails. But looks not to have anything to do with my modifications. samples\Microsoft.AspNet.SignalR.ServiceSample\Startup.cs is using some MapAzureSignalR method, but it is nowhere defined in the original repository.

https://github.com/SignalR/SignalR/search?q=MapAzureSignalR

@analogrelay
Copy link
Contributor

Hrm strange. I'll take a look at it and resolve it. You're correct, it's not part of your change, it's something from the more recent builds.

@analogrelay
Copy link
Contributor

Looks like the Azure SignalR SDK team cleared out their nightly feed, but fortunately they've also published a preview of their package. Once #4218 is merged, we can rebase this on master and the CI should be fixed.

There are also some flaky functional tests (which I've been slogging through de-flaking) that may fail, so if the CI goes red, it may not mean we can't merge it 😄

@analogrelay
Copy link
Contributor

It should be possible to rebase this on master now and get the CI build running properly. Are you able to do that @zorgoz ?

@analogrelay analogrelay added the more-info-needed We are currently waiting for a response. No further triage action is needed at this time. label Oct 5, 2018
@analogrelay
Copy link
Contributor

Closing this as we haven't heard from you. If you're still interested in getting this merged in, feel free to comment and we can revisit it!

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
more-info-needed We are currently waiting for a response. No further triage action is needed at this time.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

3 participants